Enable runtime closure verification

[NikolaMilosavljevic]

Fixes: #31805

Overriding Arcade target to allow closure verification without other validations and to allow us to incorporate a fix for a bug in VerifyClosure. As discovered during fix verification, the task would fail if a destination folder (for DGML file) does not exist - issue in Arcade: dotnet/arcade#6090

[ghost]

Tagging subscribers to this area: @safern, @ViktorHofer
See info in area-owners.md if you want to be subscribed.

[ericstj]

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]

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]

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]

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

[NikolaMilosavljevic]

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]

@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]

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

[NikolaMilosavljevic]

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.

[ericstj]

If I understand correctly the current state of this PR is not running the target and you need to include the new package in order to fix the condition. Is that correct?

[NikolaMilosavljevic]

If I understand correctly the current state of this PR is not running the target and you need to include the new package in order to fix the condition. Is that correct?

Yes, that's correct - this PR needs to be completed by 9/4, but Arcade change will come a bit later.

[NikolaMilosavljevic]

If I understand correctly the current state of this PR is not running the target and you need to include the new package in order to fix the condition. Is that correct?

@ericstj I don't mind copying VerifyClosure target from Arcade and removing it later. We already need to remove the other target once VerifyClosure task issue is fixed.

If we do this, we ensure that target runs and we're not blocked by package update.

[ericstj]

I see, one thing I've done in the past is manually update a single package once signed build is available (which is typically an hour or two after check-in). Is that not possible?

[NikolaMilosavljevic]

I see, one thing I've done in the past is manually update a single package once signed build is available (which is typically an hour or two after check-in). Is that not possible?

Yeah - I've added that change now.

[ericstj]

This looks good. Did you confirm the target is running?

[NikolaMilosavljevic]

This looks good. Did you confirm the target is running?

Yes - confirmed in my local build, but will check PR validation build as well, once it completes.

[NikolaMilosavljevic]

This looks good. Did you confirm the target is running?

Yes, the target is running. However, I might need to update this PR. Double-checking right now, but I might need to pick a new version of another package, due to some refactoring done about a month ago, by @mmitche : dotnet/arcade@4864664#diff-af7c096fa15e403c04f3343d8a820d57

[NikolaMilosavljevic]

This looks good. Did you confirm the target is running?

Yes, the target is running. However, I might need to update this PR. Double-checking right now, but I might need to pick a new version of another package, due to some refactoring done about a month ago, by @mmitche : dotnet/arcade@4864664#diff-af7c096fa15e403c04f3343d8a820d57

Confirmed that the target is running in validation builds.

Also confirmed that dependent intra-Arcade package is properly restored during build.

[NikolaMilosavljevic]

/backport to release/5.0-rc2

[github-actions]

Started backporting to release/5.0-rc2: https://github.com/dotnet/runtime/actions/runs/238245693

[github-actions]

@NikolaMilosavljevic backporting to release/5.0-rc2 failed, the patch most likely resulted in conflicts:

$ git am --3way --ignore-whitespace --keep-non-patch changes.patch

Applying: Enable closure validation
Applying: Ensure dependency graph directory exists
Applying: Netstandard should not be ignored
Applying: Ensure Reports dir exists; VerifyClosure change moved to Arcade
Applying: Upgrade SharedFramework.SDK package to pick up a change for VerifyClosure target
error: sha1 information is lacking or useless (eng/Version.Details.xml).
error: could not build fake ancestor
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0005 Upgrade SharedFramework.SDK package to pick up a change for VerifyClosure target
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!