Make sure Microsoft.CodeAnalysis.Collections builds with net6.0

[sharwell]

No description provided.

[benvillalobos]

Thanks for the quick response 👍

[benvillalobos]

Pinging this, I think dotnet/msbuild#6148 is near completion but CI needs an updated package to run tests.

[sharwell]

@dotnet/roslyn-compiler I need two reviews on this to unblock MSBuild

[jaredpar]

The Half change makes me worry that we broke consumers of this code who are using netcoreapp3.1. Want to make sure that didn't happen

[jaredpar]

The Half change makes me worry that we broke consumers of this code who are using netcoreapp3.1. Want to make sure that didn't happen

[sharwell]

@jaredpar System.Half was first added in net5.0 (dotnet/runtime#936)

[jaredpar]

Correct but this project still targets netcoreapp3.1 where the type is not defined. It's unclear how this change accounts for that.

roslyn/src/Dependencies/Collections/Microsoft.CodeAnalysis.Collections.Package.csproj

Line 5 in f77139d

<TargetFrameworks>netcoreapp3.1;netstandard2.0</TargetFrameworks>

The #if NET is an attempt to guard it but I'm unsure when it triggers as it's not part of the standard defines.

Or put another way if the #if NET were replaced with #if NET5_0_OR_GREATER the change would make sense to me. But as is I don't see how this both

1. Compiles when targeting netcoreapp3.1
2. Compiles and does Half logic when targeting net5.0 or higher

[sharwell]

This is a source content package. The target framework(s) of the package project are used to validate successful compilation but are not used downstream.

[jaredpar]

The target framework(s) of the package project are used to validate successful compilation but are not used downstream.

If so then why aren't we adding net6.0 to the target frameworks list here?

Still haven't answered what #if NET is or how it triggers diff for netcoreapp3.1 and net6.0. That seems important to this change.

[sharwell]

If so then why aren't we adding net6.0 to the target frameworks list here?

I tried, but the Correctness Rebuild leg can't handle it. The cause was not clear, and has been left as a follow-up to expedite unblocking MSBuild.

Still haven't answered what #if NET is or how it triggers diff for netcoreapp3.1 and net6.0. That seems important to this change.

The sections with #if NET will be dictated entirely by the consuming project. In this case, dotnet/msbuild is targeting net6.0 and these sections will be active.

[jaredpar]

I tried, but the Correctness Rebuild leg can't handle it. The cause was not clear, and has been left as a follow-up to expedite unblocking MSBuild.

What was the error and where is the tracking issue?

The sections with #if NET will be dictated entirely by the consuming project. In this case, dotnet/msbuild is targeting net6.0 and these sections will be active.

Is it up to them to define NET? If so when do they define NET? It's a non-standard define so there should be docs explaining when it should be defined and when it should not be defined.

It's still unclear why this is used over a standard define like NET_5_0_OR_GREATER. Doing that means the right behavior occurs for free while the current change means users must do something out of the norm to get the right behavior

[sharwell]

What was the error and where is the tracking issue?

For some reason ADO isn't showing it in the history. I submitted a follow-up PR to add the target in #59187.

Is it up to them to define NET?

NET is a predefined constant for net5.0 and greater.
https://github.com/dotnet/sdk/blob/997376c6bc798af22633a8eb36074a0acec8a200/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.BeforeCommon.targets#L180

[sharwell]

Note that NET was defined by dotnet/sdk#11635 for the 5.0.0 release, and NET5_0_OR_GREATER was added later in dotnet/sdk#14798 for the 5.0.2 release.

[jaredpar]

Note that NET was defined by dotnet/sdk#11635 for the 5.0.0 release, and NET5_0_OR_GREATER was added later in dotnet/sdk#14798 for the 5.0.2 release.

Okay I see it now in the defines document. It's a sub-bullet in one of the net5.0 sections.

Searching for "NET" in that document is not a fruitful exercise 😦