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

Update ValidateExecutableReferences with new schema #19786

Conversation

Forgind
Copy link
Member

@Forgind Forgind commented Aug 17, 2021

Opts into using the new schema and looks for a TF with its first attribute equal to nearestTargetFramework.

There are multiple ways to do this, so I'm open to suggested improvements 🙂

@dsplaisted
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@dsplaisted
Copy link
Member

This is failing a bunch of tests on full framework, presumably because it's running on an older version of MSBuild which doesn't have dotnet/msbuild#6699.

Can we condition setting _UseAttributeForTargetFrameworkInfoPropertyNames on whether the MSBuild version is 17.0 or higher, and also support both schemas in ValidateExecutableReferences?

@Forgind
Copy link
Member Author

Forgind commented Sep 10, 2021

I'm confused—doesn't the SDK come with its own MSBuild? I would've thought that would mean that if your SDK is new enough to have this change, it would be new enough to have 6699, too. Had the main I based this on not been updated with 6699 yet or something?

@@ -1089,6 +1093,7 @@ Copyright (c) .NET Foundation. All rights reserved.
SelfContained="$(SelfContained)"
IsExecutable="$(_IsExecutable)"
ReferencedProjects="@(_MSBuildProjectReferenceExistent)"
MSBuildVersion="$(MSBuildVersion)"
Copy link
Member

Choose a reason for hiding this comment

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

Could we pass the value of _UseAttributeForTargetFrameworkInfoPropertyNames in here instead of the MSBuild version? Then we wouldn't have two places where we're checking whether we're on version 17 or higher.

@marcpopMSFT
Copy link
Member

@dsplaisted should this be in RC2?

@dsplaisted
Copy link
Member

@dsplaisted should this be in RC2?

Yes, I would go ahead and merge it.

@marcpopMSFT
Copy link
Member

@Forgind , can you prep for RC2 then?

@Forgind Forgind changed the base branch from main to release/6.0.1xx-rc2 September 23, 2021 23:40
@marcpopMSFT marcpopMSFT merged commit f5e3961 into dotnet:release/6.0.1xx-rc2 Sep 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants