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

rename MinimumOSPlatformAttribute -> SupportedOSPlatformAttribute #12775

Merged
merged 7 commits into from
Aug 20, 2020

Conversation

adamsitnik
Copy link
Member

@wli3 this is the follow up of dotnet/runtime#40371

I was not sure whether I should also rename the "MinimumOSPlatform" property to "SupportedOSPlatform". Any thoughts on that?

@wli3
Copy link

wli3 commented Aug 6, 2020

Yes. We do need to change the property. Could help double check it did not use anywhere else?

…sRenaming

# Conflicts:
#	src/Tests/Microsoft.NET.Build.Tests/GivenThatWeWantToBuildALibraryWithOSMinimumVersion.cs
@adamsitnik adamsitnik requested a review from a team as a code owner August 6, 2020 12:42
@adamsitnik
Copy link
Member Author

Could help double check it did not use anywhere else?

@wli3 done! please let me know if anything else needs to be changed. Thanks!

@wli3
Copy link

wli3 commented Aug 6, 2020

The next runtime insertion with the type change will be blocked the failure tests. And by when we need to cherry-pick these changes to the runtime insertion PR to unblock it.

@wli3
Copy link

wli3 commented Aug 12, 2020

For some reason it did not work. The new runtime is in but, it still fail to build

@adamsitnik
Copy link
Member Author

For some reason it did not work. The new runtime is in but, it still fail to build

could you please point me to the logs or paste the full error message here?

@wli3
Copy link

wli3 commented Aug 13, 2020

@adamsitnik the current master should have the type rename flow in already. The fact that this PR still fail to build is the problem.

It should have something to do with how SDK build and test. But I haven't get time to look into it yet.

@adamsitnik
Copy link
Member Author

It should have something to do with how SDK build and test.

I've synced my branch and was able to reproduce the problem locally.

For some reason, there are two .NET 5 versions being downloaded:

  • 5.0.0-rc.1.20371.13 (uses the old name)
  • 5.0.0-rc.1.20410.10 (uses the new name)

The question is: how can we force the tooling to use the newer version?

@wli3
Copy link

wli3 commented Aug 13, 2020

It is not really what SDK to install. Since the new installed SDK won't have the type change (get stuck here dotnet/installer#8129). During SDK test, it should use the latest runtime and reference assemblies of the new runtime. But for some reason it doesn't hence the missing type error.

@@ -117,11 +117,11 @@ Copyright (c) .NET Foundation. All rights reserved.
</ItemGroup>

<ItemGroup Condition="'$(TargetPlatformIdentifier)' != ''
and '$(MinimumOSPlatform)' != ''
Copy link
Contributor

Choose a reason for hiding this comment

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

@adamsitnik \ @wli3 do you happen to know what values I should use to make this work for Blazor? What values do I need to specify in the SDK for TargetPlatformIdentifier and MinimumOSPlatform?

@adamsitnik
Copy link
Member Author

During SDK test, it should use the latest runtime and reference assemblies of the new runtime. But for some reason it doesn't hence the missing type error.

FWIW I've taken a look at the problem and the dotnet process that is running the unit tests itself (the host process) is using the latest bits (5.0.0-rc.1.20410.10) and knows what SupportedOSPlatformAttribute type is.

The dotnet process spawned by the tests to build the generated test projects (sdk\artifacts\bin\redist\Release\dotnet\dotnet.exe run in my example) is using the old bits (5.0.0-rc.1.20371.13) and it fails to compile the project generated by the tests.

The sdk\artifacts\bin\redist\Release\dotnet\shared\Microsoft.NETCore.App contains 5.0.0-rc.1.20410.10 folder, so whichever setting tells dotnet host proces to use Microsoft.NETCore.App=5.0.0-rc.1.20410.10 is not applied to the dotnet process spawned by the tests.

@wli3
Copy link

wli3 commented Aug 14, 2020

@adamsitnik thank you! that's great investigation. That's very close to getting a solution. Sorry I've fire fighting the preview 8 release

@wli3
Copy link

wli3 commented Aug 17, 2020

I think I have a plan. The BundledNETCoreAppPackageVersion still uses the stage 0 runtime. So it builds against the last version. I plan to just override the version with XML in C# at

<OverlaySdkFilesFromStage0 Include="$(_DotNetHiveRoot)/sdk/$(Stage0SdkVersion)/Microsoft.NETCoreSdk.BundledVersions.props" />

Use a task to override since it was generated as a string literal replace anyway. And using C# can have better error when anything goes wrong. cc @dsplaisted

@wli3 wli3 self-requested a review August 19, 2020 20:19
Use the runtime in dotnet/sdk instead of in the stage 0 to avoid
circular dependency.
If there is a change depended on the latest runtime. Without override
the runtime version in BundledNETCoreAppPackageVersion
we would need to somehow get this change in without the test, and then
insertion dotnet/installer
and then update the stage 0 back.

Use a task to override since it was generated as a string literal
replace anyway.
And using C# can have better error when anything goes wrong.
@wli3 wli3 merged commit 5cb7e1f into dotnet:master Aug 20, 2020
@jeffhandley
Copy link
Member

jeffhandley commented Aug 20, 2020

Merged #12775 into master.

Great work, @wli3 and @adamsitnik!

@adamsitnik
Copy link
Member Author

@wli3 thanks a lot!

@adamsitnik adamsitnik deleted the platformAttributesRenaming branch August 20, 2020 04:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants