Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 27 commits into from
Feb 1, 2023
Merged

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

Show file tree
Hide file tree
Changes from 25 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
02e8b58
Copy local intellisense xmls for assemblies with source of truth.
carlossanlop Dec 2, 2022
3227c67
use UseIntellisenseDocumentationFile instead
smasher164 Jan 3, 2023
175deee
Create intellisense.targets, which contains the Target that defines t…
carlossanlop Jan 10, 2023
33d63d8
Set UseIntellisenseDocumentationFile to true for the 3 assemblies tha…
carlossanlop Jan 10, 2023
4cdf926
Delete docs.targets, move package download to Build.proj
carlossanlop Jan 20, 2023
4785850
Consume $(XmlDocDir) in Microsoft.NetCore.App.Ref.sfxproj
carlossanlop Jan 20, 2023
27ae4a6
Change condition to import intellisense.targets: Use $(IsSourceProjec…
carlossanlop Jan 20, 2023
d5cbf96
Move XmlDocFileRoot to intellisense.targets, address suggestions for …
carlossanlop Jan 20, 2023
15ff2cd
Expand condition of CopyDocumentationFileToXmlDocDir to also check if…
carlossanlop Jan 20, 2023
4d3633d
Change AfterTargets of CopyDocumentationFileToXmlDocDir from CoreComp…
carlossanlop Jan 20, 2023
74a4b4e
Move PackageDownload to intellisense.targets
carlossanlop Jan 26, 2023
19249f5
Remove Choose+When. Readjust properties for Private.Intellisense file…
carlossanlop Jan 26, 2023
83773b7
Rename csproj property to something more clear.
carlossanlop Jan 27, 2023
ba8ac9a
Default csproj property to true.
carlossanlop Jan 27, 2023
8c39ea9
Missed adding the "not previously set" condition for UseIntellisenseP…
carlossanlop Jan 27, 2023
6197945
Remove incorrect condition in CopyDocumentationFileToXmlDocDir.
carlossanlop Jan 27, 2023
4d94daa
Missed property reuse in Condition in sfxproj
carlossanlop Jan 27, 2023
80d0d0d
Only evaluate second IntellisensePackageXmlFile if the first one did …
carlossanlop Jan 27, 2023
d2a60f9
Move intellisense.targets import from root to libraries, right after …
carlossanlop Jan 27, 2023
f4ce3cb
Enable CS1591, skip arcade warning
carlossanlop Jan 27, 2023
2c7048e
Move SkipArcadeNoWarnCS1591 to src/libraries/Directory.Build.props to…
carlossanlop Jan 27, 2023
459ce39
Remove unnecessary slashes in sfxproj
carlossanlop Jan 27, 2023
2d70d61
Move SkipArcadeNoWarn up to the top of src/libraries/Directory.Build.…
carlossanlop Jan 27, 2023
c689a05
Revert Brotli and Vectors csproj changes. Those assemblies are either…
carlossanlop Jan 31, 2023
4d3e550
Add target to intellisense.targets that runs as InitialTarget of the …
carlossanlop Jan 31, 2023
981d201
Improve error message: Mention the problematic property and the offen…
carlossanlop Feb 1, 2023
dc8ef9a
Added extra condition to verification target to only run when UseInte…
carlossanlop Feb 1, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 0 additions & 3 deletions Build.proj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,6 @@
</ItemGroup>

<Import Project="$(RepositoryEngineeringDir)SubsetValidation.targets" />

<!-- Upfront restore hooks -->
<Import Project="$(RepositoryEngineeringDir)restore\docs.targets" />
<Import Project="$(RepositoryEngineeringDir)restore\optimizationData.targets" Condition="'$(DotNetBuildFromSource)' != 'true'" />

<Target Name="BuildLocalTasks"
Expand Down
1 change: 0 additions & 1 deletion Directory.Build.props
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,6 @@
<IbcOptimizationDataDir>$([MSBuild]::NormalizeDirectory('$(ArtifactsDir)', 'ibc'))</IbcOptimizationDataDir>
<MibcOptimizationDataDir>$([MSBuild]::NormalizeDirectory('$(ArtifactsDir)', 'mibc'))</MibcOptimizationDataDir>
<XmlDocDir>$([MSBuild]::NormalizeDirectory('$(ArtifactsBinDir)', 'docs'))</XmlDocDir>
<XmlDocFileRoot>$([MSBuild]::NormalizeDirectory('$(NuGetPackageRoot)', 'microsoft.private.intellisense', '$(MicrosoftPrivateIntellisenseVersion)', 'IntellisenseFiles', 'net'))</XmlDocFileRoot>
<DocsDir>$([MSBuild]::NormalizeDirectory('$(MSBuildThisFileDirectory)', 'docs'))</DocsDir>
<ManPagesDir>$([MSBuild]::NormalizeDirectory('$(DocsDir)', 'manpages'))</ManPagesDir>

Expand Down
43 changes: 43 additions & 0 deletions eng/intellisense.targets
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
<Project InitialTargets="VerifyAssemblySupportsDocsXmlGeneration">

<PropertyGroup>
<UseIntellisensePackageDocXmlFile Condition="'$(UseIntellisensePackageDocXmlFile)' == ''">true</UseIntellisensePackageDocXmlFile>
<NoWarn Condition="'$(UseIntellisensePackageDocXmlFile)' == 'true'">$(NoWarn);1591</NoWarn>
<IntellisensePackageXmlRootFolder>$([MSBuild]::NormalizeDirectory('$(NuGetPackageRoot)', 'microsoft.private.intellisense', '$(MicrosoftPrivateIntellisenseVersion)', 'IntellisenseFiles'))</IntellisensePackageXmlRootFolder>
<IntellisensePackageXmlFileFromNetFolder>$([MSBuild]::NormalizePath('$(IntellisensePackageXmlRootFolder)', 'net', '1033', '$(AssemblyName).xml'))</IntellisensePackageXmlFileFromNetFolder>
<IntellisensePackageXmlFileFromDotNetPlatExtFolder>$([MSBuild]::NormalizePath('$(IntellisensePackageXmlRootFolder)', 'dotnet-plat-ext', '1033', '$(AssemblyName).xml'))</IntellisensePackageXmlFileFromDotNetPlatExtFolder>
<IntellisensePackageXmlFile Condition="'$(UseIntellisensePackageDocXmlFile)' == 'true' and Exists($(IntellisensePackageXmlFileFromNetFolder))">$(IntellisensePackageXmlFileFromNetFolder)</IntellisensePackageXmlFile>
<IntellisensePackageXmlFile Condition="'$(IntellisensePackageXmlFile)' == '' and '$(UseIntellisensePackageDocXmlFile)' == 'true' and Exists($(IntellisensePackageXmlFileFromDotNetPlatExtFolder))">$(IntellisensePackageXmlFileFromDotNetPlatExtFolder)</IntellisensePackageXmlFile>
</PropertyGroup>

<Target Name="VerifyAssemblySupportsDocsXmlGeneration">
carlossanlop marked this conversation as resolved.
Show resolved Hide resolved
<Error Condition="'$(UseIntellisensePackageDocXmlFile)' == 'false' and '$(IsPartialFacadeAssembly)' == 'true'" Text="Cannot currently generate full xml documentation for an assembly that is a partial facade." />
carlossanlop marked this conversation as resolved.
Show resolved Hide resolved
carlossanlop marked this conversation as resolved.
Show resolved Hide resolved
<Error Condition="'$(UseIntellisensePackageDocXmlFile)' == 'false' and '$(GeneratePlatformNotSupportedAssemblyMessage)' != ''" Text="Cannot currently generate full xml documentation for an assembly that generates PNSE." />
carlossanlop marked this conversation as resolved.
Show resolved Hide resolved
</Target>

<ItemGroup>
<PackageDownload Include="Microsoft.Private.Intellisense" Version="[$(MicrosoftPrivateIntellisenseVersion)]" />
</ItemGroup>

<!-- TODO: Remove this target when no library relies on the intellisense documentation file anymore.-->
<!-- Replace the default xml file generated in the obj folder with the one that comes from the docs feed. -->
<Target Name="ChangeDocumentationFileForPackaging"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@ViktorHofer ViktorHofer Jan 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added some tests in a comment in the main thread.

AfterTargets="DocumentationProjectOutputGroup"
carlossanlop marked this conversation as resolved.
Show resolved Hide resolved
Condition="'$(UseIntellisensePackageDocXmlFile)' == 'true'">
<ItemGroup>
<DocFileItem Remove="@(DocFileItem)" />
<DocFileItem Include="$(IntellisensePackageXmlFile)" />
</ItemGroup>
</Target>

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I created an issue to track this: #82191

DependsOnTargets="ChangeDocumentationFileForPackaging">
<Copy SourceFiles="@(DocFileItem)"
DestinationFolder="$(XmlDocDir)"
SkipUnchangedFiles="true"
UseHardlinksIfPossible="true" />
</Target>

</Project>
14 changes: 1 addition & 13 deletions eng/packaging.targets
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,9 +28,7 @@
<GeneratePackageOnBuild Condition="'$(GeneratePackageOnBuild)' != 'true' and
'$(BuildAllConfigurations)' == 'true' and
'$(DotNetBuildFromSource)' == 'true'">true</GeneratePackageOnBuild>
<!-- Search for the documentation file in the intellisense package and otherwise pick up the generated one. -->
<LibIntellisenseDocumentationFilePath>$(XmlDocFileRoot)1033\$(AssemblyName).xml</LibIntellisenseDocumentationFilePath>
<UseIntellisenseDocumentationFile Condition="'$(UseIntellisenseDocumentationFile)' == '' and Exists('$(LibIntellisenseDocumentationFilePath)')">true</UseIntellisenseDocumentationFile>

ViktorHofer marked this conversation as resolved.
Show resolved Hide resolved
<!-- During NoBuild pack invocations, skip project reference build. Necessary for the IncludeProjectReferencesWithPackAttributeInPackage target. -->
<BuildProjectReferences Condition="'$(NoBuild)' == 'true'">false</BuildProjectReferences>
</PropertyGroup>
Expand Down Expand Up @@ -108,16 +106,6 @@
($(TargetFrameworks.Contains('$(NetFrameworkMinimum)')) or $(TargetFrameworks.Contains('net47')) or $(TargetFrameworks.Contains('net48')))" />
</ItemGroup>

<!-- TODO: Remove this target when no library relies on the intellisense documentation file anymore.-->
ViktorHofer marked this conversation as resolved.
Show resolved Hide resolved
<Target Name="ChangeDocumentationFileForPackaging"
AfterTargets="DocumentationProjectOutputGroup"
Condition="'$(UseIntellisenseDocumentationFile)' == 'true'">
<ItemGroup>
<DocumentationProjectOutputGroupOutput Remove="@(DocumentationProjectOutputGroupOutput)" />
<DocumentationProjectOutputGroupOutput Include="$(LibIntellisenseDocumentationFilePath)" />
</ItemGroup>
</Target>

<!-- Add runtime specific file into the package if the tfm is RID specific. -->
<Target Name="AddRuntimeSpecificFilesToPackage"
DependsOnTargets="BuiltProjectOutputGroup;
Expand Down
46 changes: 0 additions & 46 deletions eng/restore/docs.targets
View file
Open in desktop

This file was deleted.

2 changes: 1 addition & 1 deletion src/installer/pkg/sfx/Microsoft.NETCore.App/Microsoft.NETCore.App.Ref.sfxproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
<Target Name="AddFrameworkFilesToPackage" DependsOnTargets="ResolveLibrariesFromLocalBuild" BeforeTargets="GetFilesToPackage">
<ItemGroup>
<ReferencePath Include="@(LibrariesRefAssemblies)" />
<DocFilesToPackage Include="$(ArtifactsBinDir)/docs/%(LibrariesRefAssemblies.FileName).xml" Condition="Exists('$(ArtifactsBinDir)/docs/%(LibrariesRefAssemblies.FileName).xml')"/>
<DocFilesToPackage Include="$(XmlDocDir)%(LibrariesRefAssemblies.FileName).xml" Condition="Exists('$(XmlDocDir)%(LibrariesRefAssemblies.FileName).xml')"/>
<Analyzer Include="$(MicrosoftNetCoreAppRefPackDir)/analyzers/**/*.*" />
<FilesToPackage Include="@(Analyzer)" ExcludeFromValidation="true" TargetPath="analyzers/%(RecursiveDir)" />
</ItemGroup>
Expand Down
5 changes: 5 additions & 0 deletions src/libraries/Directory.Build.props
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,11 @@
<PropertyGroup>
<SkipInferTargetOSName>true</SkipInferTargetOSName>
<DisableArcadeTestFramework>true</DisableArcadeTestFramework>
<!-- Enabling this rule will cause build failures on undocumented public APIs.
We cannot add it in eng/Versions.props because src/coreclr does not have access to UseIntellisensePackageDocXmlFile, which ensures
we only enable it in specific projects. so to avoid duplicating this property in coreclr, we can first scope it to src/libraries.
This property needs to be declared before the ..\..\Directory.Build.props import. -->
<SkipArcadeNoWarnCS1591>true</SkipArcadeNoWarnCS1591>
</PropertyGroup>

<Import Project="..\..\Directory.Build.props" />
Expand Down
1 change: 1 addition & 0 deletions src/libraries/Directory.Build.targets
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@
</PropertyGroup>

<Import Project="$(RepositoryEngineeringDir)versioning.targets" />
<Import Project="$(RepositoryEngineeringDir)intellisense.targets" Condition="'$(IsSourceProject)' == 'true'" />

<!-- Libraries-specific binplacing properties -->
<PropertyGroup>
Expand Down
1 change: 1 addition & 0 deletions src/libraries/System.Formats.Cbor/src/System.Formats.Cbor.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
<TargetFrameworks>$(NetCoreAppCurrent);$(NetCoreAppPrevious);$(NetCoreAppMinimum);netstandard2.0;$(NetFrameworkMinimum)</TargetFrameworks>
<AllowUnsafeBlocks>true</AllowUnsafeBlocks>
<IsPackable>true</IsPackable>
<UseIntellisensePackageDocXmlFile>false</UseIntellisensePackageDocXmlFile>
carlossanlop marked this conversation as resolved.
Show resolved Hide resolved
<PackageDescription>Provides classes that can read and write the CBOR data format.

Commonly Used Types:
Expand Down