Copy local intellisense xmls for assemblies with source of truth. #79134

carlossanlop · 2022-12-02T03:13:19Z

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 pack command also included the correct intellisense file in the nupkgs.

ghost · 2022-12-02T03:13:32Z

Tagging subscribers to this area: @dotnet/area-infrastructure-libraries
See info in area-owners.md if you want to be subscribed.

Issue Details

Tentative 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.

Author:	carlossanlop
Assignees:	carlossanlop, smasher164
Labels:	`area-Infrastructure-libraries`
Milestone:	-

smasher164 · 2022-12-02T23:15:58Z

The CI errors seem to be happening because it's trying to copy the XMLs over on configurations besides -allconfigurations, which shouldn't happen. Maybe the docsFile target runs even when not in allconfigurations?

carlossanlop · 2022-12-02T23:22:22Z

Our assumption was that we only want this xml copy to happen when -allconfigurations is executed because when we do not build -allconfigurations, the only folder that gets created for many assemblies is the net7.0-windows one, but we need the non-OS specific one for this to work. We could be wrong about our approach, though.

If we confirm we only want this copy to happen in -allconfigurations, then we need to find a way to detect that the user passed -allconfigurations and use it as a condition to run our target.

ghost · 2023-01-02T05:01:49Z

Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it.

Directory.Build.props

Directory.Build.targets

eng/packaging.targets

eng/restore/docs.targets

src/libraries/System.Formats.Cbor/src/System.Formats.Cbor.csproj

eng/intellisense.targets

ericstj · 2023-01-12T00:17:31Z

Things to test to make sure this is behaving correctly:

Library nuget package is honoring this flag and packing source built XML vs doc-team provided XML. Check both cases.
Microsoft.NETCore.App targeting pack is honoring this flag. Check both cases.
Transport packages are honoring this flag. Check both cases.

I don't think any of these are broken, but I'm not sure about the last one. It would be good to check.

Directory.Build.targets

ViktorHofer · 2023-01-12T13:09:50Z

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 <SkipArcadeNoWarnCS1591>true</SkipArcadeNoWarnCS1591> in Versions.props and then set <NoWarn Condition="'$(UseIntellisenseDocumentationFile)' != 'true'">$(NoWarn);1591</NoWarn> in intellisense.targets to not break existing projects that haven't yet annotated all their public API.

eng/intellisense.targets

Directory.Build.props

eng/intellisense.targets

ericstj · 2023-01-20T18:40:36Z

eng/intellisense.targets

+  </Choose>
+
+  <!-- TODO: Remove this target when no library relies on the intellisense documentation file anymore.-->
+  <Target Name="ChangeDocumentationFileForPackaging"


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.

Here's a sample that does this: ericstj@94849ba

Did you verify that this works in a pack-only scenario as well? dotnet pack --no-build

Added some tests in a comment in the main thread.

carlossanlop · 2023-01-20T22:27:26Z

I just submitted dotnet/arcade#12188 to make it possible to have a compiler error for NoWarn=CS1591

Thanks, @ViktorHofer. I'll rebase the PR.

Build.proj

…props to ensure it gets loaded before importing arcade.

carlossanlop · 2023-01-27T20:12:12Z

Pending trying to find out why Brotli is only generating SR files (Viktor is helping me).

ericstj · 2023-01-27T21:38:37Z

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.

ViktorHofer · 2023-01-28T10:50:16Z

Pending trying to find out why Brotli is only generating SR files (Viktor is helping me).

Broti only has SR resources because the net8.0 tfm is a PNSE assembly:

runtime/src/libraries/System.IO.Compression.Brotli/src/System.IO.Compression.Brotli.csproj

Line 9 in 7d4163f

    
           <GeneratePlatformNotSupportedAssemblyMessage Condition="'$(TargetPlatformIdentifier)' == ''">SR.IOCompressionBrotli_PlatformNotSupported</GeneratePlatformNotSupportedAssemblyMessage>

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

migrate existing PNSE auto-generated code into runtime condition (i.e. if (OperatingSystem.IsWindows) ..., else if (...), else { throw PNSE(); }
or if we want to keep building for different platforms, use compiler preprocessor directives:

/// <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.

ViktorHofer · 2023-01-28T11:00:35Z

from @ericstj:

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.

It would be great if we could find a solution that would contribute to #58163 as well:

from #58163:

It's yet unclear how to generate these type forwards without having a reference assembly available that represents the public API surface area. A possible solution to that could be to check in the type forwards, adding them to the compilation input and making sure that they are kept up to date. The problem with that is that Roslyn keeps passed-in type forward attributes in reference assemblies instead of following the type and replacing the attribute with the actual type. We should sync up with the Roslyn team to see if supporting this scenario is feasible.

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:

The NetCoreAppCurrent configuration uses the GeneratePlatformNotSupportedMessage feature or
The NetCoreAppCurrent configuration uses the IsPartialFacadeAssembly feature.

safern · 2023-01-30T18:05:33Z

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:

PNSE assemblies which use the GeneratePlatformNotSupportedMessage feature.
Facade assemblies which have at least one typeforward.
Cross RID assemblies which have different runtime implementations.

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 facade assemblies:
- get all the typeforwards generated (maybe choose the built asset for the matching TargetOS or the Host OS, since we are in an outerbuild), get the generated XML file with the docs for all it's references and get the matching docs for the types that were forwarded and move them over to the facade assembly's main doc xml.
- I think this could also be done in a source generator, but that would run for all inner builds, but might be "simpler" implementation wise?
- I don't know if this could be a Roslyn feature that could be built via an opt-in switch, something like, GenerateXmlDocsForTypeForward and also how feasible since Roslyn would need to resolve the docs for the forwarded types.
For cross RID assemblies we agreed that we were going to turn on the doc generation and also the warning that all public API must have a triple slash document so that we ensure all the OSs implementations had triple slash comments, however we need to include some validation to make sure we produce the same XML output regardless of the OS it is being built on, or this could be optimized to only be done when in a local developer build or an all configuration build to not affect the CI verticals. The task would do something like:
- Validate all xml files for all OS builds are the same. I don't know if this could just do a git diff (that would require all contributors to have git installed on the path, but would perhaps be simpler and more efficiente than doing it on an MSBuild Task? It could also be text diff if that meets the criteria.
- Choose any produced xml file (perhaps the one applicable to the Target OS/Host OS) as we already ensured that all of them are the same.

For the PNSE scenario I see 2 options:

Check-in the XML file with all the documentation for the APIs and perhaps add validation that all the public APIs in the assembly are present in the XML file.
As Viktor mentioned above, remove the magic PNSE generator (that would potentially be needed if we ever wanted to get RID of the ref assemblies' source code) and check-in the source code that throws the PNSE explicitly, if it does for all OSs then a simple PNSE source file, if not, it could be included via compile conditions on OS/TFM and then the checked-in source code would need to have the triple slash comments.

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 IsPartialFacadeAssembly and don't use GeneratePlatformNotSupportedMessage.

src/libraries/System.Formats.Cbor/src/System.Formats.Cbor.csproj

ViktorHofer · 2023-01-31T18:27:34Z

@carlossanlop as we discussed offline, please add a validation target in intellisense.targets that emits an error if UseIntellisensePackageDocXmlFile is false (means that the project opts into using the compiler produced xml file) and the project either sets IsPartialFacadeAssembly to true or when GeneratePlatformNotSupportedAssemblyMessage isn't empty as we currently don't support projects that use these features.

… 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.

carlossanlop · 2023-01-31T21:46:06Z

Below is a test of the latest 2 commits.

The error message shows up in assemblies where I temporarily set UseIntellisensePackageDocXmlFile to false, and they are either partial facades or use PNSE. The only one that does not have neither of those conditions is Cbor. It's the first result, built successfully, and is the only one we will merge with UseIntellisensePackageDocXmlFile set to false with this PR.

PS D:\runtime\src\libraries> dotnet build .\System.Formats.Cbor\src\System.Formats.Cbor.csproj -f net8.0 /bl 
  MSBuild version 17.5.0-preview-23067-01+771199907 for .NET
    Determining projects to restore...
    All projects are up-to-date for restore.
    Microsoft.Interop.SourceGeneration -> D:\runtime\artifacts\bin\Microsoft.Interop.SourceGeneration\Debug\netstandard2.0\Microsoft.Interop.SourceGeneration.dll
    LibraryImportGenerator -> D:\runtime\artifacts\bin\LibraryImportGenerator\Debug\netstandard2.0\Microsoft.Interop.LibraryImportGenerator.dll
    System.Formats.Cbor -> D:\runtime\artifacts\bin\System.Formats.Cbor\ref\Debug\net8.0\System.Formats.Cbor.dll
    System.Formats.Cbor -> D:\runtime\artifacts\bin\System.Formats.Cbor\Debug\net8.0\System.Formats.Cbor.dll

  Build succeeded.
      0 Warning(s)
      0 Error(s)

  Time Elapsed 00:00:01.31

PS D:\runtime\src\libraries> dotnet build .\System.Numerics.Vectors\src\System.Numerics.Vectors.csproj -f net8.0 /bl          
  MSBuild version 17.5.0-preview-23067-01+771199907 for .NET
  D:\runtime\eng\intellisense.targets(14,5): error : Cannot currently generate full xml documentation for an assembly that is a partial facade. [D:\runtime\src\libraries\System.Numeric
  s.Vectors\src\System.Numerics.Vectors.csproj]

PS D:\runtime\src\libraries> dotnet build .\System.IO\src\System.IO.csproj -f net8.0 /bl          
  MSBuild version 17.5.0-preview-23067-01+771199907 for .NET
  D:\runtime\eng\intellisense.targets(14,5): error : Cannot currently generate full xml documentation for an assembly that is a partial facade. [D:\runtime\src\libraries\System.IO\src\
  System.IO.csproj]

PS D:\runtime\src\libraries> dotnet build .\System.Formats.Tar\src\System.Formats.Tar.csproj -f net8.0 /bl  
  MSBuild version 17.5.0-preview-23067-01+771199907 for .NET
  D:\runtime\eng\intellisense.targets(15,5): error : Cannot currently generate full xml documentation for an assembly that generates PNSE. [D:\runtime\src\libraries\System.Formats.Tar\
  src\System.Formats.Tar.csproj]

PS D:\runtime\src\libraries> dotnet build .\System.IO.Compression.Brotli\src\System.IO.Compression.Brotli.csproj -f net8.0 /bl
  MSBuild version 17.5.0-preview-23067-01+771199907 for .NET
  D:\runtime\eng\intellisense.targets(15,5): error : Cannot currently generate full xml documentation for an assembly that generates PNSE. [D:\runtime\src\libraries\System.IO.Compressi
  on.Brotli\src\System.IO.Compression.Brotli.csproj]

PS D:\runtime\src\libraries> dotnet build .\System.IO.Ports\src\System.IO.Ports.csproj -f net8.0 /bl                
  MSBuild version 17.5.0-preview-23067-01+771199907 for .NET
  D:\runtime\eng\intellisense.targets(15,5): error : Cannot currently generate full xml documentation for an assembly that generates PNSE. [D:\runtime\src\libraries\System.IO.Ports\src
\System.IO.Ports.csproj]

PS D:\runtime\src\libraries> dotnet build .\System.Drawing.Common\src\System.Drawing.Common.csproj -f net8.0 /bl
  MSBuild version 17.5.0-preview-23067-01+771199907 for .NET
  D:\runtime\eng\intellisense.targets(14,5): error : Cannot currently generate full xml documentation for an assembly that is a partial facade. [D:\runtime\src\libraries\System.Drawing
  .Common\src\System.Drawing.Common.csproj]

eng/intellisense.targets

…ding assembly.

eng/intellisense.targets

…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.

carlossanlop · 2023-02-01T22:28:14Z

CI failure is unrelated and pre-existing: #48798
The manual tests in various csproj with and without enabling the new property, worked as expected.
Signed-off, so I'm merging now.

ViktorHofer · 2023-02-10T16:25:45Z

eng/intellisense.targets

+
+  <Target Name="CopyDocumentationFileToXmlDocDir"
+          AfterTargets="CopyFilesToOutputDirectory"
+          Condition="'$(IsNetCoreAppSrc)' == 'true' and '$(TargetFramework)' == '$(NetCoreAppCurrent)'"


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.

I created an issue to track this: #82191

carlossanlop added the area-Infrastructure-libraries label Dec 2, 2022

carlossanlop requested review from jeffhandley, ViktorHofer and ericstj December 2, 2022 03:13

carlossanlop assigned carlossanlop and smasher164 Dec 2, 2022

ghost closed this Jan 2, 2023

carlossanlop reopened this Jan 10, 2023

carlossanlop force-pushed the DocsXml branch from 3061de8 to 3ff1428 Compare January 10, 2023 03:41

carlossanlop commented Jan 10, 2023

View reviewed changes

carlossanlop marked this pull request as ready for review January 10, 2023 17:20

ericstj reviewed Jan 11, 2023

View reviewed changes

eng/intellisense.targets Outdated Show resolved Hide resolved

ericstj reviewed Jan 11, 2023

View reviewed changes

eng/intellisense.targets Outdated Show resolved Hide resolved

ericstj reviewed Jan 12, 2023

View reviewed changes

eng/intellisense.targets Outdated Show resolved Hide resolved

ViktorHofer reviewed Jan 12, 2023

View reviewed changes

Directory.Build.targets Outdated Show resolved Hide resolved

ViktorHofer reviewed Jan 12, 2023

View reviewed changes

eng/intellisense.targets Outdated Show resolved Hide resolved

ViktorHofer reviewed Jan 12, 2023

View reviewed changes

eng/intellisense.targets Outdated Show resolved Hide resolved

ViktorHofer reviewed Jan 12, 2023

View reviewed changes

Directory.Build.props Outdated Show resolved Hide resolved

smasher164 reviewed Jan 20, 2023

View reviewed changes

eng/intellisense.targets Outdated Show resolved Hide resolved

ericstj reviewed Jan 20, 2023

View reviewed changes

carlossanlop force-pushed the DocsXml branch from 69909c2 to 87ab04e Compare January 20, 2023 22:28

This comment was marked as outdated.

Sign in to view

ViktorHofer reviewed Jan 23, 2023

View reviewed changes

Build.proj Outdated Show resolved Hide resolved

carlossanlop added 2 commits January 27, 2023 12:08

Remove unnecessary slashes in sfxproj

459ce39

Move SkipArcadeNoWarn up to the top of src/libraries/Directory.Build.…

2d70d61

…props to ensure it gets loaded before importing arcade.

carlossanlop commented Jan 31, 2023

View reviewed changes

src/libraries/System.Formats.Cbor/src/System.Formats.Cbor.csproj Show resolved Hide resolved

carlossanlop added 2 commits January 31, 2023 13:44

Revert Brotli and Vectors csproj changes. Those assemblies are either…

c689a05

… a partial facade or use PNSE.

Add target to intellisense.targets that runs as InitialTarget of the …

4d3e550

…current project, and throws errors if the UseIntellisensePackageDocXmlFile is set to false and the assembly is either a partial facade or uses PNSE.

carlossanlop requested review from ViktorHofer and ericstj January 31, 2023 22:07

smasher164 reviewed Feb 1, 2023

View reviewed changes

eng/intellisense.targets Outdated Show resolved Hide resolved

eng/intellisense.targets Outdated Show resolved Hide resolved

eng/intellisense.targets Outdated Show resolved Hide resolved

Improve error message: Mention the problematic property and the offen…

981d201

…ding assembly.

This was referenced Feb 1, 2023

Test failure Loader\\classloader\\DictionaryExpansion\\DictionaryExpansion\\DictionaryExpansion.cmd #75244

Closed

Roslyn source generator crash on mono/linux/arm64 #81123

Closed

ViktorHofer reviewed Feb 1, 2023

View reviewed changes

eng/intellisense.targets Outdated Show resolved Hide resolved

ViktorHofer approved these changes Feb 1, 2023

View reviewed changes

carlossanlop mentioned this pull request Feb 1, 2023

Test_EventSource_EtwManifestGeneration* tests failing in CI #48798

Closed

carlossanlop merged commit 0b3703e into dotnet:main Feb 1, 2023

carlossanlop deleted the DocsXml branch February 1, 2023 22:29

ViktorHofer reviewed Feb 10, 2023

View reviewed changes

carlossanlop mentioned this pull request Feb 15, 2023

Newly added intellisense msbuild conditions cause overbuild #82191

Closed

ViktorHofer mentioned this pull request Feb 17, 2023

Some OOB assemblies are not copying intellisense extracted from docs package #82214

Closed

carlossanlop mentioned this pull request Mar 16, 2023

Change execution order conditions for intellisense swapping and copying msbuild targets. #83117

Merged

ghost locked as resolved and limited conversation to collaborators Mar 18, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Copy local intellisense xmls for assemblies with source of truth. #79134

Copy local intellisense xmls for assemblies with source of truth. #79134

carlossanlop commented Dec 2, 2022 •

edited

Loading

ghost commented Dec 2, 2022

smasher164 commented Dec 2, 2022

carlossanlop commented Dec 2, 2022

ghost commented Jan 2, 2023

ericstj commented Jan 12, 2023

ViktorHofer commented Jan 12, 2023

ericstj Jan 20, 2023

ericstj Jan 23, 2023

ViktorHofer Jan 24, 2023 •

edited

Loading

carlossanlop Jan 27, 2023

carlossanlop commented Jan 20, 2023

This comment was marked as outdated.

carlossanlop commented Jan 27, 2023

ericstj commented Jan 27, 2023

ViktorHofer commented Jan 28, 2023 •

edited

Loading

ViktorHofer commented Jan 28, 2023 •

edited

Loading

safern commented Jan 30, 2023

ViktorHofer commented Jan 31, 2023 •

edited

Loading

carlossanlop commented Jan 31, 2023

carlossanlop commented Feb 1, 2023

ViktorHofer Feb 10, 2023

carlossanlop Feb 15, 2023

Copy local intellisense xmls for assemblies with source of truth. #79134

Copy local intellisense xmls for assemblies with source of truth. #79134

Conversation

carlossanlop commented Dec 2, 2022 • edited Loading

ghost commented Dec 2, 2022

smasher164 commented Dec 2, 2022

carlossanlop commented Dec 2, 2022

ghost commented Jan 2, 2023

ericstj commented Jan 12, 2023

ViktorHofer commented Jan 12, 2023

ericstj Jan 20, 2023

Choose a reason for hiding this comment

ericstj Jan 23, 2023

Choose a reason for hiding this comment

ViktorHofer Jan 24, 2023 • edited Loading

Choose a reason for hiding this comment

carlossanlop Jan 27, 2023

Choose a reason for hiding this comment

carlossanlop commented Jan 20, 2023

This comment was marked as outdated.

carlossanlop commented Jan 27, 2023

ericstj commented Jan 27, 2023

ViktorHofer commented Jan 28, 2023 • edited Loading

ViktorHofer commented Jan 28, 2023 • edited Loading

safern commented Jan 30, 2023

ViktorHofer commented Jan 31, 2023 • edited Loading

carlossanlop commented Jan 31, 2023

carlossanlop commented Feb 1, 2023

ViktorHofer Feb 10, 2023

Choose a reason for hiding this comment

carlossanlop Feb 15, 2023

Choose a reason for hiding this comment

carlossanlop commented Dec 2, 2022 •

edited

Loading

ViktorHofer Jan 24, 2023 •

edited

Loading

ViktorHofer commented Jan 28, 2023 •

edited

Loading

ViktorHofer commented Jan 28, 2023 •

edited

Loading

ViktorHofer commented Jan 31, 2023 •

edited

Loading