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

Change netcoreapp5.0 to net5.0 #35176

Merged
merged 8 commits into from
May 6, 2020
Merged

Conversation

ViktorHofer
Copy link
Member

@ViktorHofer ViktorHofer commented Apr 19, 2020

No Merge. Part of batched rollout in May: #35202

Contributes towards dotnet/sdk#10756
Requires dotnet/arcade#5292

I will create a doc after merging this which describes when to use which condition. Basically the recommendation is:

  1. Use an equality check if the TargetFramework isn't overloaded with the OS portion.
  2. Use a StartsWith when you want to test for .NETStandard or .NETFramework
  3. Use a StartsWith if the TargetFramework is overloaded with the OS portion.
  4. Use negations if that makes the conditions easier.

cc @ericstj

@ghost
Copy link

ghost commented Apr 19, 2020

Tagging subscribers to this area: @ViktorHofer
Notify danmosemsft if you want to be subscribed.

@Anipik
Copy link
Contributor

Anipik commented Apr 19, 2020

We will need to change our custom parsing for the designTimeBuilds as well.

@ViktorHofer ViktorHofer added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Apr 20, 2020
@ViktorHofer
Copy link
Member Author

We will need to change our custom parsing for the designTimeBuilds as well.

Master already has the net5.0 parsing logic: https://github.com/dotnet/runtime/blob/master/src/libraries/intellisense.targets#L42-L54, dotnet/sdk@0f5fc4b

@@ -16,7 +16,7 @@
<Reference Include="System.Text.Json" />
</ItemGroup>

<ItemGroup Condition="$(TargetFramework.StartsWith('netcoreapp'))">
<ItemGroup Condition="$(TargetFramework.StartsWith('$(NetCoreAppCurrent)'))">
Copy link
Member

Choose a reason for hiding this comment

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

Should this instead test for equality?

Copy link
Member Author

@ViktorHofer ViktorHofer May 4, 2020

Choose a reason for hiding this comment

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

No, because the TargetFramework is overloaded with the OS: netcoreapp5.0-Windows_NT.
I will create a doc after merging this which describes when to use what. Basically the recommendation is:

  1. Use an equality check if the TargetFramework isn't overloaded with the OS portion.
  2. Use a StartsWith when you want to test for multiple .NETStandard or .NETFramework versions
  3. Use a StartsWith if the TargetFramework is overloaded with the OS portion.
  4. Use negations if that makes the conditions easier.

@@ -14,8 +14,8 @@
<PropertyGroup>
<!-- For the inbox library (that is shipping with the product), this should always be true. -->
<!-- BUILDING_INBOX_LIBRARY is only false when building the netstandard compatible NuGet package. -->
<DefineConstants Condition="$(TargetFramework.StartsWith('netcoreapp'))">$(DefineConstants);BUILDING_INBOX_LIBRARY</DefineConstants>
<NoWarn Condition="!$(TargetFramework.StartsWith('netcoreapp'))">$(NoWarn);nullable</NoWarn>
<DefineConstants Condition="'$(TargetFramework)' == '$(NetCoreAppCurrent)' or '$(TargetFramework)' == 'netcoreapp3.0'">$(DefineConstants);BUILDING_INBOX_LIBRARY</DefineConstants>
Copy link
Member

Choose a reason for hiding this comment

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

What about netcoreapp3.1?

Copy link
Member

Choose a reason for hiding this comment

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

We don’t build System.Text.Json for netcoreapp3.1 and we shouldn’t need as it has been inbox since netcoreapp3.0.

Also it seems like we shouldn’t need to cross build System.Text.Json for netcoreapp3.0 since it is inbox and the package should just contain an inbox placeholder:

https://github.com/dotnet/runtime/blob/master/src/libraries/pkg/baseline/packageIndex.json#L6253

Copy link
Member Author

Choose a reason for hiding this comment

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

We should do a pass over all our OOBs at some point and make sure that we only build what's actually necessary. This will help us keep our build configuration and conditions sane.

@ViktorHofer ViktorHofer added auto-merge and removed NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) labels May 6, 2020
@ghost
Copy link

ghost commented May 6, 2020

Hello @ViktorHofer!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@ViktorHofer ViktorHofer merged commit 000046f into dotnet:master May 6, 2020
@ViktorHofer ViktorHofer deleted the Net5.0 branch May 6, 2020 11:53
@ghost ghost locked as resolved and limited conversation to collaborators Dec 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants