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.

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

SetPlatform Negotiation: Allow MSBuild GetTargetFrameworks call when SetTargetFramework already set #6724

Merged
merged 2 commits into from
Aug 12, 2021

Conversation

benvillalobos
Copy link
Member

@benvillalobos benvillalobos commented Aug 2, 2021

Context

Fix potential issue where an already-set 'SetTargetFramework' metadata on a ProjectReference item would skip logic required for SetPlatform negotiation.

When SkipGetTargetFrameworkProperties is true, it skips the MSBuild call to GetTargetFrameworks. I modified a condition to ensure it wouldn't be set to true when opted into the feature, but just below that was this:

    <ItemGroup>
      <_MSBuildProjectReferenceExistent Condition="'%(_MSBuildProjectReferenceExistent.SetTargetFramework)' != ''">
        <SkipGetTargetFrameworkProperties>true</SkipGetTargetFrameworkProperties>
      </_MSBuildProjectReferenceExistent>
    </ItemGroup>

So if we skipped setting SkipGetTargetFrameworkProperties to true, it would still be set to true if SetTargetFramework was set. This would skip the call to GetTargetFrameworks, which would prevent the proper metadata being added to the item before GetCompatiblePlatform is called, likely resulting in a warning and the project being built with no metadata

Changes Made

Added a condition not to set SkipGetTargetFrameworkProperties when opted into platform negotiation.

Testing

Not tested yet.

Notes

One of those "wake up in the middle of the night and this hits you" moments.

metadata would skip the MSBuild call to the GetTargetFrameworks target.
This would prevent the proper metadata being added to the item before
GetCompatiblePlatform is called, likely resulting in a warning and
the project being built with no metadata
@benvillalobos
Copy link
Member Author

benvillalobos commented Aug 3, 2021

Tested locally on a managed project reference to an unmanaged project.

This is what happens when SetTargetFramework is set already
image

With the fix:
image
Notice SetTargetFramework is set on the project item in the MSBuild task call and the task call still happens.

The result: Even when SetTargetFramework is set, when we're opted into platform negotiation we still get the GetTargetFrameworks MSBuild call that platform negotiation needs.

@benvillalobos benvillalobos marked this pull request as ready for review August 3, 2021 21:25
Copy link
Member

@Forgind Forgind left a comment

Choose a reason for hiding this comment

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

Sounds reasonable

@Forgind Forgind added the merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now. label Aug 9, 2021
@rokonec rokonec merged commit 62c6327 into dotnet:main Aug 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants