Fixing overbuilding of platform negotiated projects#8106
Merged
rokonec merged 13 commits intodotnet:mainfrom Nov 3, 2022
MIchaelRShea:main
Merged
Fixing overbuilding of platform negotiated projects#8106rokonec merged 13 commits intodotnet:mainfrom MIchaelRShea:main
rokonec merged 13 commits intodotnet:mainfrom
MIchaelRShea:main
Conversation
benvillalobos
suggested changes
Oct 31, 2022
| } | ||
| // If the referenced project has a defined `Platform` that's compatible, it will build that way by default. | ||
| // Don't set `buildProjectReferenceAs` and the `_GetProjectReferencePlatformProperties` target will handle the rest. | ||
| if (!string.IsNullOrEmpty(referencedProjectPlatform) && referencedProjectPlatform.Equals(buildProjectReferenceAs, StringComparison.OrdinalIgnoreCase)) |
Member
There was a problem hiding this comment.
Context: This if statement translates to "Are we about to tell the referenced project to build using its default? Then don't." It's a catch-all check that gets around the other ways in which users could end up in this situation.
Member
|
Also needs a test 😁 |
benvillalobos
suggested changes
Oct 31, 2022
Member
benvillalobos
left a comment
There was a problem hiding this comment.
Can you add a test that looks like ResolvesViaAnyCPUDefault, but the referenced project has AnyCPU as its default platform? This should result in an empty NearestPlatform, instead of passing AnyCPU.
Note: ResolvesViaAnyCPUDefault doesn't need to be changed, because that captures the scenario where AnyCPU isn't the default platform, but we default to it because there is no match, but AnyCPU is contained in Platforms
Co-authored-by: Ben Villalobos <4691428+BenVillalobos@users.noreply.github.com>
Co-authored-by: Ben Villalobos <4691428+BenVillalobos@users.noreply.github.com>
Co-authored-by: Ben Villalobos <4691428+BenVillalobos@users.noreply.github.com>
benvillalobos
added a commit
to benvillalobos/setplatform-repro
that referenced
this pull request
Oct 31, 2022
rainersigwald
approved these changes
Nov 2, 2022
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Context
Platform negotiation still ends up in situations where project A explicitly tells project B to build as B's default platform. This leads to an unnecessary global property being passed, which leads to an unnecessary evaluation, and in some cases an overbuild.
This PR adds a catch-all that empties out the global Platform property whenever we fall into this case.
Changes Made
Add catch-all if statement that prevents global property Platform to be passed to referenced projects when redundant (the project would have built as that platform anyway).
Testing
Notes
This will unblock the implementation of this feature in VS. the only thing currently blocking adoption of this in VS is our datacenter build(cloudbuild) graph validation. it sees two project nodes with different platform global properties evaluating to the same configuration and throws an error to prevent race conditions.