Fix cache issues with graph exempt projects #5222

cdmihai · 2020-04-01T21:44:36Z

This PR fixes the bug cascade that results when exempting projects from graph isolation that also exist in the graph. It implements this region of the docs: https://github.com/microsoft/msbuild/compare/master...cdmihai:cacheAndIsolationDocumentation?expand=1#diff-5de82af4d8f52787d69b38fd58c509caR44

When a reference is exempt from isolation constraints, there can be multiple situations:

the reference does not exist in the graph, or in the input caches files. This is what the original exemption feature was designed for, and it worked great for enabling WPF's on the fly generated projects work with isolation. All the following scenarios were not supported, and fixed in this PR.
the reference exists in one or more input cache files. This causes part of the failures in Projects excluded from static graph constraint checks should not appear in the caches #4386. There are multiple sub cases here:
- the reference is in the input cache, but its build results are not a superset of the current project's requests. This happens when the exempt project is in the input cache with results for other targets than the ones requested by the current project.
- the reference is in the input cache and is a superset of the current project's requests.

Things this PR changes:

If the CacheAggregator detects conflicting incoming cache entries (conflicting instances of BuldRequestConfiguration and BuildResult), instead of erroring it will now merge them using the "first entry wins" strategy. BuildResults are merged at the level of individual target results. This allows multiple projects to exempt the same reference.
A BuildRequestConfiguration is marked as exempt (new bool field) whenever a building project marks it as exempt.
The override caches (ConfigCacheWithOverride, BuildResultsWithOverride) always ensure an entry is either in the override cache, or the current cache, but not in both.
a. An exception here is the BuildResultsWithOverride cache, where an exempt configuration can appear in both caches, This happens when the exempt project is present in the input caches, but does not have target results that the current project wants to build. In this case, two things happen. In the ConfigWithOverride cache, the entry is migrated from the override cache to the current cache. In the BuildResultsWithOverride cache, The override entry gets the new, additional targets (not ideal, but it would be super hard to change), and the current cache gets only the newly built targets, to ensure the invariant that the current cache should only contain newly built things, and should never contain results from deserialized caches..

cdmihai · 2020-04-03T20:37:35Z

src/Build/BackEnd/BuildManager/BuildManager.cs

@@ -511,22 +513,24 @@ void InitializeCaches()
                _configCache = ((IBuildComponentHost)this).GetComponent(BuildComponentType.ConfigCache) as IConfigCache;
                _resultsCache = ((IBuildComponentHost)this).GetComponent(BuildComponentType.ResultsCache) as IResultsCache;

-                if (!usesInputCaches && (_buildParameters.ResetCaches || _configCache.IsConfigCacheSizeLargerThanThreshold()))
+                if (usesInputCaches)


Affects non-isolation execution: just a condition simplification, should be benign.

cdmihai · 2020-04-03T20:41:28Z

src/Build/BackEnd/Shared/BuildRequestConfiguration.cs

@@ -547,6 +548,12 @@ internal int ResultsNodeId
            set => _resultsNodeId = value;
        }

+        public bool SkippedFromStaticGraphIsolationConstraints


Affects non-isolation execution: new translatable bool that never gets set or read outside isolation execution

cdmihai · 2020-04-03T20:43:05Z

I've commented on all the diffs that affect non-isolation code paths, you can search for them with the string Affects non-isolation execution:

benvillalobos · 2020-04-02T21:28:20Z

scripts/Deploy-MSBuild.ps1

@@ -36,58 +51,65 @@ if ($runtime -eq "Desktop") {
    $targetFramework = "netcoreapp2.1"
 }

+$bootstrapBinDirectory = "artifacts\bin\MSBuild.Bootstrap\$configuration\$targetFramework"


What's the difference between artifacts\bin\MSBuild.Bootstrap\... vs. artifacts\bin\bootstrap\...?

Honestly I do not know. I also saw that, but envisioned the hell of trying to understand the diffs so I didn't get into it. @rainersigwald, do you know more? :)

benvillalobos · 2020-04-02T21:38:38Z

scripts/Deploy-MSBuild.ps1

    )
 }

 $filesToCopyToBin += $runtimeSpecificFiles

 foreach ($file in $filesToCopyToBin) {
-    Copy-WithBackup $([IO.Path]::Combine($PSScriptRoot, "..", $file))
+    Copy-WithBackup $file
 }

 Write-Host -ForegroundColor Green "Copy succeeded"


Would it sense to break the script change into its own PR?

Transitive project references are a thing now. Added support to static graph, so that buildxl and qb can avoid adding the transitive refs. This PR is independent of #5222. Ideally, review this one first, as QB has a stronger dependency on this PR than on #5222. Design - transitive references are opt-in, per project evaluation - once a project opts-in, transitivity is applied for all ProjectReference items - a project opt-ins by setting the property AddTransitiveProjectReferencesInStaticGraph to true. The sdk does this automatically in Microsoft.Managed.After.Targets. - interaction with crosstargeting: transitive refs are added only to inner builds, not the outer builds. This mimics vanilla msbuild. Co-authored-by: Rainer Sigwald <raines@microsoft.com>

* LOC CHECKIN | microsoft/msbuild vs16.6 | 20200420 (#5299) * Final branding_16.6 (#5273) * merge * Enable handshake timeout on Mono (#5318) I am seeing consistent test hangs on macOS Mono. A node is getting into a bad state and is failing to respond to handshakes. While I do not yet understand the root cause, it is clear that having a timeout on the handshake operation mitigates the issue. The test is now failing but does not hang, saving developer time as well as test pipeline resources. * Revert "Reverted and rebuilt for localizable strings." (#5246) This adds back support for logging an error when a task returns false without logging an error. This was originally added in #4940 but was reverted because of multiple difficulties. * Changed error code * Add escape hatch * Fix typo * Filtering for duplicate content files with referencing projects (#4931) * Updating content filtering based on content copying changes * Add a flag that is enabled by default on Core; otherwise disabled by default. * Adding Shutdown Message to Worker Nodes (#5262) * Changed where Trace is being called and removed old functionality. * Updating .NET Core branding to .NET (#5286) * Override default Arcade Xunit configuration (#5302) * Update Directory.Build.targets * prevent arcade from injecting its own xunit file * Don't log @(ReferencePath) at the end of ImplicitlyExpandDesignTimeFacades (#5317) The actual ItemGroups inside the target already do a good job of logging exactly what items were added and/or removed and in what order and with what metadata. Emitting an extra low-pri message which is unstructured here just adds noise, slows the builds down, wastes binlog space and is otherwise redundant. * Compute hashes in parallel in GetFileHash (#5303) * Compute hashes in parallel. This scales better for larger number of files. * Use a dedicated write lock * Allow disabling logging of task parameters and item metadata (#5268) This enables fine-grained control over whether: * to log log each parameter (whether input or output) * or whether to log item metadata for each ITaskItem[] parameter. When LogTaskInputs is set the default behavior is still to log all parameters and all item metadata for ITaskItem[] parameters. Since this is very verbose and hurts performance without adding any useful information it is valuable to be able to turn this logging off in certain situations. This approach allows controlling logging via setting simple properties or environment variables. I've identified the specific tasks and parameters that we want to restrict logging for that would give us the most gains without losing any significant useful info: https://github.com/KirillOsenkov/MSBuildStructuredLog/wiki/Task-Parameter-Logging * Update dependencies from https://github.com/dotnet/arcade build 20200430.5 (#5325) - Microsoft.DotNet.Arcade.Sdk: 1.0.0-beta.20221.2 -> 1.0.0-beta.20230.5 Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com> * Use environment variable for handshake Resolves #4961 (#5196) * Use environment variable for handshake Resolves #4961 * Combine means of hashing * Support transitive project references in static graph (#5326) Transitive project references are a thing now. Added support to static graph, so that buildxl and qb can avoid adding the transitive refs. This PR is independent of #5222. Ideally, review this one first, as QB has a stronger dependency on this PR than on #5222. Design - transitive references are opt-in, per project evaluation - once a project opts-in, transitivity is applied for all ProjectReference items - a project opt-ins by setting the property AddTransitiveProjectReferencesInStaticGraph to true. The sdk does this automatically in Microsoft.Managed.After.Targets. - interaction with crosstargeting: transitive refs are added only to inner builds, not the outer builds. This mimics vanilla msbuild. Co-authored-by: Rainer Sigwald <raines@microsoft.com> Co-authored-by: Martin Chromecek (Moravia IT) <v-chmart@microsoft.com> Co-authored-by: Ladi Prosek <laprosek@microsoft.com> Co-authored-by: Sarah Oslund <sfoslund@microsoft.com> Co-authored-by: Ben Villalobos <villalobosb93@gmail.com> Co-authored-by: Mihai Codoban <codobanmihai@gmail.com> Co-authored-by: Kirill Osenkov <KirillOsenkov@users.noreply.github.com> Co-authored-by: Pranav K <prkrishn@hotmail.com> Co-authored-by: dotnet-maestro[bot] <42748379+dotnet-maestro[bot]@users.noreply.github.com> Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com> Co-authored-by: Rainer Sigwald <raines@microsoft.com>

Transitive project references are a thing now. Added support to static graph, so that buildxl and qb can avoid adding the transitive refs. This PR is independent of dotnet#5222. Ideally, review this one first, as QB has a stronger dependency on this PR than on dotnet#5222. Design - transitive references are opt-in, per project evaluation - once a project opts-in, transitivity is applied for all ProjectReference items - a project opt-ins by setting the property AddTransitiveProjectReferencesInStaticGraph to true. The sdk does this automatically in Microsoft.Managed.After.Targets. - interaction with crosstargeting: transitive refs are added only to inner builds, not the outer builds. This mimics vanilla msbuild. Co-authored-by: Rainer Sigwald <raines@microsoft.com>

rainersigwald · 2020-05-21T16:24:22Z

Talked to @cdmihai offline; the plan is to go a different direction.

cdmihai force-pushed the exemptionBug branch from 8f0fcbc to 37c5a9f Compare April 1, 2020 22:03

cdmihai commented Apr 3, 2020

View reviewed changes

benvillalobos reviewed Apr 6, 2020

View reviewed changes

This was referenced Apr 8, 2020

Update static graph docs #5259

Closed

Copy satellites in deployment script #5260

Merged

This was referenced Apr 16, 2020

[Static graph] Support transitive project references in static graph #5287

Closed

[Static graph] Support skipping nonexisting targets #5297

Closed

cdmihai added 11 commits April 20, 2020 13:27

Simplify condition

46294df

Tests

fd71db1

Resolve config collisions from exempt projects

e21922b

Copy satellites in deployment script

c8f67ea

Don't add new cache entries in the cache of old results

10cfc85

Change debug assertions to mandatory assertions

ff8227a

Handle case where exempt project also exists in the input results cache

019b954

Merge colliding configs

75d5874

Update debug assertions

3ba0c76

Mark self build configs as exempt from isolation constraints

726b29f

Rename

890360d

cdmihai force-pushed the exemptionBug branch from 37c5a9f to 890360d Compare April 20, 2020 21:52

rainersigwald closed this May 21, 2020

cdmihai added a commit to cdmihai/msbuild that referenced this pull request Jan 16, 2021

Remove paragraph about the fixes in dotnet#5222

30ef9ff

cdmihai mentioned this pull request Jan 16, 2021

Update static graph docs #6043

Merged

cdmihai mentioned this pull request Feb 25, 2021

Projects excluded from static graph constraint checks should not appear in the caches #4386

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix cache issues with graph exempt projects #5222

Fix cache issues with graph exempt projects #5222

cdmihai commented Apr 1, 2020 •

edited

Loading

cdmihai Apr 3, 2020

cdmihai Apr 3, 2020

cdmihai commented Apr 3, 2020

benvillalobos Apr 2, 2020

cdmihai Apr 8, 2020

benvillalobos Apr 2, 2020

rainersigwald commented May 21, 2020

Fix cache issues with graph exempt projects #5222

Fix cache issues with graph exempt projects #5222

Conversation

cdmihai commented Apr 1, 2020 • edited Loading

cdmihai Apr 3, 2020

Choose a reason for hiding this comment

cdmihai Apr 3, 2020

Choose a reason for hiding this comment

cdmihai commented Apr 3, 2020

benvillalobos Apr 2, 2020

Choose a reason for hiding this comment

cdmihai Apr 8, 2020

Choose a reason for hiding this comment

benvillalobos Apr 2, 2020

Choose a reason for hiding this comment

rainersigwald commented May 21, 2020

cdmihai commented Apr 1, 2020 •

edited

Loading