Skip to content

Enable runtime closure verification #41698

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

Merged
NikolaMilosavljevic merged 8 commits into from
Sep 3, 2020
Merged

Enable runtime closure verification #41698

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
65656e7
Enable closure validation
NikolaMilosavljevic Aug 31, 2020
58cf696
Ensure dependency graph directory exists
NikolaMilosavljevic Sep 1, 2020
d4395af
Netstandard should not be ignored
NikolaMilosavljevic Sep 2, 2020
7a42449
Ensure Reports dir exists; VerifyClosure change moved to Arcade
NikolaMilosavljevic Sep 2, 2020
d184666
Merge branch 'verify.closure' of https://github.com/nikolamilosavljev…
NikolaMilosavljevic Sep 2, 2020
52d24f5
Upgrade SharedFramework.SDK package to pick up a change for VerifyClo…
NikolaMilosavljevic Sep 3, 2020
2df9482
Merge branch 'master' into verify.closure
NikolaMilosavljevic Sep 3, 2020
47aeaf8
Merge branch 'master' into verify.closure
NikolaMilosavljevic Sep 3, 2020
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
32 changes: 32 additions & 0 deletions src/installer/pkg/projects/Directory.Build.targets
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,38 @@
</ItemGroup>
</Target>

<!--
Overriding VerifyClosure target to enable closure validation without other validations.

@ericstj ericstj Sep 1, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If this is just temporary rather than forking the whole thing, just have a single target:

<Target Name="EnsureDepenedencyGraphDir" BeforeTargets="VerifyClosure">
    <MakeDir Directories="$(PackageReportDir)$(Id)" />
</Target>

@NikolaMilosavljevic NikolaMilosavljevic Sep 1, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I actually need to override the target to update it's condition as well, to run if new property is set - ShouldVerifyClosure. Currently VerifyClosure only runs if SkipValidatePackage is false: https://github.com/dotnet/arcade/blob/e11b8486ad155fab13e788a268755c0ca60d3609/src/Microsoft.DotNet.Build.Tasks.SharedFramework.Sdk/targets/framework.packaging.targets#L170

@ericstj ericstj Sep 2, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is there a reason we can’t get the arcade update instead? Would it take too long? Could you manually update just the package with this fix?

@NikolaMilosavljevic NikolaMilosavljevic Sep 2, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I've heard that there is a freeze on whole Arcade update, but I'll ask if a single package would be allowed.

@NikolaMilosavljevic NikolaMilosavljevic Sep 2, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Heard back - we can get update in RC2. I will update VerifyClosure target in Microsoft.DotNet.Build.Tasks.SharedFramework.Sdk in Arcade, and remove the target override from my PR. But, I will pick your suggestion for a new target to create Reports directory, which works around an issue in VerifyClosure task: dotnet/arcade#6090. The task is in a different Arcade package and they want to minimize the number of changes now.

@NikolaMilosavljevic NikolaMilosavljevic Sep 2, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@ericstj I will also move your suggested new target into Arcade next to VerifyClosure target. We can, then, remove this target when VerifyClosure task is fixed - with a single PR in Arcade.

@NikolaMilosavljevic NikolaMilosavljevic Sep 2, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Will keep EnsureDepenedencyGraphDir in runtime repo - PackageReportDir is runtime-specific property, it doesn't make sense for this to be in Arcade.

@NikolaMilosavljevic NikolaMilosavljevic Sep 2, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Arcade fix is in: dotnet/arcade#6101

This (runtime) fix can be merged and closure validation would start working once we pick up new Microsoft.DotNet.Build.Tasks.SharedFramework.Sdk package.

@ericstj @jkoritzinsky this is safe to merge once you approve the changes.

-->
<Target Name="VerifyClosure"
DependsOnTargets="GetClosureFiles"
Condition="'$(SkipValidatePackage)' != 'true' or '$(ShouldVerifyClosure)' == 'true'"
Inputs="%(ClosureFile.FileSet)"
Outputs="batching-on-FileSet-metadata">
<ItemGroup>
<_closureFileNames Include="@(ClosureFile->'%(FileName)')" Original="%(Identity)" />

<_closureFileNamesFiltered Include="@(_closureFileNames)" Exclude="@(ExcludeFromClosure)"/>
<_closureFileFiltered Include="@(_closureFileNamesFiltered->'%(Original)')"/>
</ItemGroup>

<Message Importance="High" Text="Verifying closure of $(Id) %(ClosureFile.FileSet) assemblies" />

<PropertyGroup>
<_DependencyGraphFilePath>$(PackageReportDir)$(Id)$(NuspecSuffix)-%(ClosureFile.FileSet).dgml</_DependencyGraphFilePath>
<_DependencyGraphDir>$([System.IO.Path]::GetDirectoryName($(_DependencyGraphFilePath)))</_DependencyGraphDir>
</PropertyGroup>

<!-- Ensure directory for dependency graph file exists. -->
<MakeDir Condition="!Exists('$(_DependencyGraphDir')"
Directories="$(_DependencyGraphDir)" />

<VerifyClosure
Sources="@(_closureFileFiltered)"
IgnoredReferences="@(IgnoredReference)"
DependencyGraphFilePath="$(_DependencyGraphFilePath)" />
</Target>

<!--
Finds symbol files and injects them into the package build.
-->
Expand Down
23 changes: 23 additions & 0 deletions src/installer/pkg/projects/netcoreapp/pkg/legacy/Microsoft.NETCore.App.pkgproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,31 @@
-->
<BuildLineupPackage>false</BuildLineupPackage>

<ShouldVerifyClosure>true</ShouldVerifyClosure>

<FrameworkListTargetPath>data/</FrameworkListTargetPath>
<SkipBuildOnRuntimePackOnlyOS>true</SkipBuildOnRuntimePackOnlyOS>
</PropertyGroup>

<ItemGroup>
<!-- netstandard will have cycles because OOB packages target netstandard and are used in netstandard closure,
We do ensure that netstandard inside the shared framework has no cycles, but we permit them in packages in order to allow for simpler netstandard-based builds.-->
<IgnoredReference Include="netstandard" />
Comment thread
jkoritzinsky marked this conversation as resolved.
Outdated
<!-- windows.winmd is not part of the framework -->
<IgnoredReference Include="Windows" />

<!-- Exclude shims from the closure verification -->
<ExcludeFromClosure Include="mscorlib" />
<ExcludeFromClosure Include="System" />
<ExcludeFromClosure Include="System.Configuration" />
<ExcludeFromClosure Include="System.Core" />
<ExcludeFromClosure Include="System.Data" />
<ExcludeFromClosure Include="System.Drawing" />
<ExcludeFromClosure Include="System.Net" />
<ExcludeFromClosure Include="System.Security" />
<ExcludeFromClosure Include="System.ServiceModel.Web" />
<ExcludeFromClosure Include="System.ServiceProcess" />
<ExcludeFromClosure Include="System.Transactions" />
<ExcludeFromClosure Include="WindowsBase" />
</ItemGroup>
</Project>