Skip to content

Remove opt in for new schema for CombineTargetFrameworkInfoProperties#6928

Merged
rokonec merged 6 commits into
dotnet:mainfrom
Forgind:remove-escape-hatch-for-validating-schema
Apr 11, 2022
Merged

Remove opt in for new schema for CombineTargetFrameworkInfoProperties#6928
rokonec merged 6 commits into
dotnet:mainfrom
Forgind:remove-escape-hatch-for-validating-schema

Conversation

@Forgind

@Forgind Forgind commented Oct 8, 2021

Copy link
Copy Markdown
Contributor

For CombineTargetFrameworkInfoProperties, we had hit an issue in which a TF could be invalid as an XML root. We changed the schema to let us escape it, then opted into the new schema in the SDK. This removes the option to not opt into the new schema. It's hopefully still ok to do this; though it would have been better to get it into 17.0, I don't think anyone other than the SDK is using this, so I don't think it should matter.

I didn't really test this, but it looks hard to mess up.

Edit: well, maybe 😛

For CombineTargetFrameworkInfoProperties, we had hit an issue in which a TF could be invalid as an XML root. We changed the schema to let us escape it, then opted into the new schema in the SDK. This removes the option to not opt into the new schema. It's hopefully still ok to do this; though it would have been better to get it into 17.0, I don't think anyone other than the SDK is using this, so I don't think it should matter.
Comment thread src/Tasks/Microsoft.Common.CurrentVersion.targets Outdated
@Forgind Forgind marked this pull request as draft October 11, 2021 14:48
@Forgind Forgind marked this pull request as ready for review February 18, 2022 17:01
Comment thread src/Tasks/CombineTargetFrameworkInfoProperties.cs
</ItemGroup>

<PropertyGroup>
<_UseAttributeForTargetFrameworkInfoPropertyNames Condition="'$(_UseAttributeForTargetFrameworkInfoPropertyNames)' == ''">false</_UseAttributeForTargetFrameworkInfoPropertyNames>

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Also put this in a change wave? I vote no because it's a private property, so anyone abusing it knows they might be broken.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Agree.

Comment thread src/Tasks/CombineTargetFrameworkInfoProperties.cs
@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 Mar 7, 2022
@rokonec rokonec merged commit fe4fde9 into dotnet:main Apr 11, 2022
@Forgind Forgind deleted the remove-escape-hatch-for-validating-schema branch April 12, 2022 16:40
rainersigwald added a commit to rainersigwald/msbuild that referenced this pull request May 20, 2022
…operties (dotnet#6928)"

This caused a regression in scenarios using Visual Studio 17.3 previews
with `global.json` files pointing to old .NET SDKs.

This reverts commit fe4fde9.

Conflicts:
	src/Framework/ChangeWaves.cs
	src/Tasks/CombineTargetFrameworkInfoProperties.cs
rokonec pushed a commit that referenced this pull request May 25, 2022
…operties (#6928)" (#7642)

This caused a regression in scenarios using Visual Studio 17.3 previews
with `global.json` files pointing to old .NET SDKs.

This reverts commit fe4fde9.

Conflicts:
	src/Framework/ChangeWaves.cs
	src/Tasks/CombineTargetFrameworkInfoProperties.cs
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.

3 participants