Skip to content

Fix TargetPlatformVersion normalization for Windows #29009

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 9 commits into from
Nov 15, 2022
Merged

Fix TargetPlatformVersion normalization for Windows #29009

merged 9 commits into from
Nov 15, 2022

Conversation

@dsplaisted
Copy link
Member

@dsplaisted dsplaisted commented Nov 10, 2022

This broke with #27199

@dotnet-maestro
Update dependencies from https://github.com/dotnet/arcade build 20221…
7d85b4f 
…013.4

Microsoft.DotNet.Arcade.Sdk , Microsoft.DotNet.Helix.Sdk , Microsoft.DotNet.SignTool , Microsoft.DotNet.XUnitExtensions
 From Version 7.0.0-beta.22511.2 -> To Version 7.0.0-beta.22513.4
@dotnet-maestro
Update dependencies from https://github.com/dotnet/arcade build 20221…
26cedb7 
…014.3

Microsoft.DotNet.Arcade.Sdk , Microsoft.DotNet.Helix.Sdk , Microsoft.DotNet.SignTool , Microsoft.DotNet.XUnitExtensions
 From Version 7.0.0-beta.22511.2 -> To Version 7.0.0-beta.22514.3
@dotnet-maestro
Update dependencies from https://github.com/dotnet/arcade build 20221…
02b89b3 
…031.4

Microsoft.DotNet.Arcade.Sdk , Microsoft.DotNet.Helix.Sdk , Microsoft.DotNet.SignTool , Microsoft.DotNet.XUnitExtensions
 From Version 7.0.0-beta.22511.2 -> To Version 7.0.0-beta.22531.4
@dotnet-maestro
Update dependencies from https://github.com/dotnet/arcade build 20221…
f8fba62 
…107.1

Microsoft.DotNet.Arcade.Sdk , Microsoft.DotNet.Helix.Sdk , Microsoft.DotNet.SignTool , Microsoft.DotNet.XUnitExtensions
 From Version 7.0.0-beta.22511.2 -> To Version 7.0.0-beta.22557.1
@dotnet-maestro
Update dependencies from https://github.com/dotnet/arcade build 20221…
d4ff0c6 
…108.4

Microsoft.DotNet.Arcade.Sdk , Microsoft.DotNet.Helix.Sdk , Microsoft.DotNet.SignTool , Microsoft.DotNet.XUnitExtensions
 From Version 7.0.0-beta.22511.2 -> To Version 7.0.0-beta.22558.4
@ghost
Copy link

ghost commented Nov 10, 2022

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@dsplaisted dsplaisted mentioned this pull request Nov 10, 2022
Merged
@dsplaisted dsplaisted force-pushed the fix-normalize-windows-platform-version branch from 48d4e91 to c610765 Compare November 10, 2022 21:46
@dsplaisted dsplaisted requested a review from a team November 11, 2022 18:11
nagilson
nagilson reviewed Nov 11, 2022
View reviewed changes
@dsplaisted dsplaisted force-pushed the fix-normalize-windows-platform-version branch 2 times, most recently from 7e7930d to b0d6e99 Compare November 11, 2022 20:04
joeloff
joeloff reviewed Nov 11, 2022
View reviewed changes
@dsplaisted
Copy link
Member Author

dsplaisted commented Nov 11, 2022

FYI @j0shuams

@dsplaisted dsplaisted changed the base branch from release/7.0.1xx to release/7.0.2xx November 14, 2022 16:58
@dsplaisted dsplaisted requested a review from a team as a code owner November 14, 2022 16:58
@dsplaisted
Fix normalization of TargetPlatformVersion for Windows
32c598b 
This broke with changes related to dotnet#27199.
It's now possible to have multiple supported platform version items for the
same version.
@dsplaisted
Fix duplicate compilation constants
edea59c
@dsplaisted
Fix Blazor baselines to account for 6.0.11
9490405
@dsplaisted
Make analyzer test package name comparison case insensitive
91b88f3 
Depending on whether we're using preview versions, some of the packages
may be downloaded by NuGet and have lowercased names, while at other
times they may be shipped in the packs folder and have mixed case.
@dsplaisted dsplaisted force-pushed the fix-normalize-windows-platform-version branch from b0d6e99 to 91b88f3 Compare November 14, 2022 17:05
@dsplaisted
Copy link
Member Author

dsplaisted commented Nov 14, 2022

This was rejected for servicing as the customer impact is low to nonexistent, so I've re-targeted it to 7.0.2xx.

@dsplaisted dsplaisted mentioned this pull request Nov 14, 2022
Closed
@dsplaisted
Copy link
Member Author

dsplaisted commented Nov 14, 2022

@nagilson Care to review this so we can merge it now?

nagilson
nagilson approved these changes Nov 14, 2022
View reviewed changes
Copy link
Member

@nagilson nagilson left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@dsplaisted I looked at the code and it seemed good, but I didn't see any test that tested this specific regression:
A TargetPlatformVersion property for Windows without a trailing “.0” won’t be normalized to 4 version sections anymore (for example from 10.0.18362 to 10.0.18362.0). Am I wrong that this is related to that?

The test changes I saw were just handling case-sensitivity, was a regression test added to check for the .0 behavior? I know some other tests (might) fail if it's wrong already.

@dsplaisted
Copy link
Member Author

dsplaisted commented Nov 15, 2022

@nagilson The test is It_succeeds_if_windows_target_platform_version_does_not_have_trailing_zeros. There's no update to it in this PR, because this PR fixes the regression that caused the test to fail. You can see it is one of the tests disabled in the 7.0.1xx PR: #28591

@dsplaisted dsplaisted merged commit 22c1f98 into dotnet:release/7.0.2xx Nov 15, 2022
@nagilson
Copy link
Member

nagilson commented Nov 15, 2022

Gotcha, thanks!

@marcpopMSFT marcpopMSFT mentioned this pull request Nov 18, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@joeloff joeloff joeloff left review comments

@nagilson nagilson nagilson approved these changes

@captainsafia captainsafia Awaiting requested review from captainsafia captainsafia is a code owner

@mkArtakMSFT mkArtakMSFT Awaiting requested review from mkArtakMSFT

Assignees

No one assigned

Labels

Servicing-rejected Test Debt

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@dsplaisted @nagilson @joeloff @MiYanni