Copy local intellisense xmls for assemblies with source of truth. #79134
Conversation
|
Tagging subscribers to this area: @dotnet/area-infrastructure-libraries Issue DetailsTentative draft. Co-authored with @smasher164 This PR explores obtaining the intellisense xml files for assemblies that have their source of truth in triple slash from the local build, and the rest of the assemblies get their intellisense xml files from the nuget package that comes from the docs repo.
|
|
The CI errors seem to be happening because it's trying to copy the XMLs over on configurations besides |
|
Our assumption was that we only want this xml copy to happen when If we confirm we only want this copy to happen in |
ghost
closed this
|
Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it. |
src/libraries/System.Formats.Cbor/src/System.Formats.Cbor.csproj
Outdated
Show resolved
Hide resolved
|
Things to test to make sure this is behaving correctly:
I don't think any of these are broken, but I'm not sure about the last one. It would be good to check. |
|
I just submitted dotnet/arcade#12188 to make it possible to have a compiler error for NoWarn=CS1591. For historical reasons that error is globally suppressed in all Arcade.Sdk consuming repositories. We should set |
| </Choose> | ||
|
|
||
| <!-- TODO: Remove this target when no library relies on the intellisense documentation file anymore.--> | ||
| <Target Name="ChangeDocumentationFileForPackaging" |
There was a problem hiding this comment.
As we just discussed, this might be too late to impact the result of a ProjectReference.
ProjectReferences (as used in the transport packages) only recieve the path to the dll in the bin folder and find the doc file relative to that.
To support ProjectReference we may want to swap the value of @(DocFileItem) instead and do so right before CopyFilesToOutputDirectory. This will make it so that the "shipping" xml is always next to the binary in the bin folder. The compiler generated one (in the case it is not shipping) will remain in the obj folder.
There was a problem hiding this comment.
Here's a sample that does this: ericstj@94849ba
There was a problem hiding this comment.
Did you verify that this works in a pack-only scenario as well? dotnet pack --no-build
There was a problem hiding this comment.
Added some tests in a comment in the main thread.
Thanks, @ViktorHofer. I'll rebase the PR. |
This comment was marked as outdated.
This comment was marked as outdated.
…props to ensure it gets loaded before importing arcade.
|
Pending trying to find out why Brotli is only generating SR files (Viktor is helping me). |
|
The compiler isn't going to emit those for you, you'll need to merge them from the dependent assembly. Maybe through a build task or source generator or something? @safern might remember some thinking around that. |
Broti only has SR resources because the net8.0 tfm is a PNSE assembly: That means that it will automatically generate public API with throwing body based on the contract/reference assembly's source. The reference assembly's source isn't XML doc annotated (by design). We will probably need a design discussion first how to handle this. In #44969, Santi talks about that problem a bit. IMHO ideally (because that would also solve some other scenarios like the ref <--> src project convergence) we would stop generating PNSE source code automatically and use checked-in code instead. I would imagine that we could either
/// <summary>
/// This is a public api method
/// </summary>
public void PublicApiMethod()
{
#if WINDOWS
WindowsImpl();
#elif UNIX
UnixImpl();
#else
throw new PlatfornNotSupportedException(SR.X);
#endif
}These are just some random thoughts from my side. I strongly support any design that moves away from auto-generated PNSE code as that will also solve other hard problems: #58163. |
|
from @ericstj:
It would be great if we could find a solution that would contribute to #58163 as well: from #58163:
Overall, I see #58163 and this effort tangled. By choosing the right design, we should be able to contribute to both efforts at the same time. That said, to not block this PR, we should be able to check-in the improvements made here by continuing to use the intellisense docs files in projects that are blocked. Blocked means that the projects uses any of these features:
|
|
Joining the party a little late but on Friday I did not get a chance to reply. When we were looking at this a couple of years ago and based on the discussion above, I think there are 3 main scenarios to consider for generating the XML files:
I think we only had an idea on how to solve for 2 and 3. For facade assemblies and Cross RID assemblies we said that we were going to add an MSBuild Task that would run ONLY on the outer build to not affect the vertical build times and also we need the outputs of all the OSs. The MSBuild Task would do something like:
For the
But I think these are incremental steps that could be done separately, and you could potentially start just having the source of truth being triple slash for assemblies that don't use |
|
@carlossanlop as we discussed offline, please add a validation target in intellisense.targets that emits an error if |
… a partial facade or use PNSE.
…current project, and throws errors if the UseIntellisensePackageDocXmlFile is set to false and the assembly is either a partial facade or uses PNSE.
|
Below is a test of the latest 2 commits. The error message shows up in assemblies where I temporarily set |
…llisense... is not set to true. Also moved the initial definition to its own solitary property group right before the verification target runs. Rename the properties that acquire the value of a file path to "FilePath" instead of just "File", for clarity.
|
CI failure is unrelated and pre-existing: #48798 |
|
|
||
| <Target Name="CopyDocumentationFileToXmlDocDir" | ||
| AfterTargets="CopyFilesToOutputDirectory" | ||
| Condition="'$(IsNetCoreAppSrc)' == 'true' and '$(TargetFramework)' == '$(NetCoreAppCurrent)'" |
There was a problem hiding this comment.
Just realized that the '$(TargetFramework)' == '$(NetCoreAppCurrent)' condition results in a behavior difference and will force us to overbuild. When building the repo, by default only the nearest NetCoreAppCurrent tfm will be built. In the case of building on Windows, this means net8.0-windows or net8.0 if projects don't have a corresponding windows platform TFM.
We should remove that artificial limitation and restore the previous behavior. I could see the following cases to apply:
- A project doesn't multi-target => use NetCoreAppCurrent condition (status quo).
- A project multi-targets and all inner builds are built (no tfm filtering) => run this target in the NetCoreAppCurrent inner build (status quo).
- A project multi-targets but an inner build runs stand-alone (tfm filtering) => Don't condition on a TFM.
I don't know if it's possible to determine the third case in an msbuild environment, but that's the default build behavior in dotnet/runtime and will be utilized in the VMR with RID builds. Therefore we should try to make that work.
There was a problem hiding this comment.
I created an issue to track this: #82191
This PR explores obtaining the intellisense xml files for assemblies that have their source of truth in triple slash from the local build, and the rest of the assemblies get their intellisense xml files from the nuget package that comes from the docs repo.
Made sure the
dotnet packcommand also included the correct intellisense file in the nupkgs.