Skip to content

Target netcoreapp3.1#40861

Merged
jaredpar merged 17 commits intodotnet:masterfrom
jaredpar:tf
Jan 31, 2020
Merged

Target netcoreapp3.1#40861
jaredpar merged 17 commits intodotnet:masterfrom
jaredpar:tf

Conversation

@jaredpar
Copy link
Member

@jaredpar jaredpar commented Jan 9, 2020

This unifies our .NET Core assets to be targeting netcoreapp3.1 where previously it was a mix of netcoreapp2.1 and netcoreapp3.0.

@jaredpar
Copy link
Member Author

@dotnet/roslyn-infrastructure PTAL

eng/build.ps1 Outdated
Copy link
Member

@tmat tmat Jan 29, 2020

Choose a reason for hiding this comment

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

This should also be unnecessary. If we need to force installation here, as opposed to when build is invoked, we can call InitializeDotNetCli.

Copy link
Member Author

Choose a reason for hiding this comment

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

Removing this breaks most of our builds

Listing running build processes...
Downloading vswhere
Building bootstrap compiler
Downloading RoslynTools.MSBuild 16.4.0-alpha
F:\workspace\_work\1\s\.tools\msbuild\16.4.0-alpha\tools\MSBuild\Current\Bin\msbuild.exe /nologo /bl:F:\workspace\_work\1\s\artifacts\log\Debug\Bootstrap.binlog /consoleloggerparameters:Verbosity=minimal;summary /m /nodeReuse:false /p:TreatWarningsAsErrors=true /p:Configuration=Release /p:UseRoslynAnalyzers=false /p:ContinuousIntegrationBuild=true /p:RoslynEnforceCodeStyle=false /p:UseRoslynAnalyzers=false /p:DotNetUseShippingVersions=true /p:InitialDefineConstants=BOOTSTRAP /p:PackageOutputPath=F:\workspace\_work\1\s\artifacts\Bootstrap /p:EnableNgenOptimization=false /p:PublishWindowsPdb=false /restore /t:Pack /warnaserror src\NuGet\Microsoft.Net.Compilers.Toolset\Microsoft.Net.Compilers.Toolset.Package.csproj
  It was not possible to find any installed .NET Core SDKs
  Did you mean to run .NET Core SDK commands? Install a .NET Core SDK from:
      https://aka.ms/dotnet-download
F:\workspace\_work\1\s\src\NuGet\Microsoft.Net.Compilers.Toolset\Microsoft.Net.Compilers.Toolset.Package.csproj : error : Unable to locate the .NET Core SDK. Check that it is installed and that the version specified in global.json (if any) matches the installed version.

Build FAILED.

F:\workspace\_work\1\s\src\NuGet\Microsoft.Net.Compilers.Toolset\Microsoft.Net.Compilers.Toolset.Package.csproj : error : Unable to locate the .NET Core SDK. Check that it is installed and that the version specified in global.json (if any) matches the installed version.
    0 Warning(s)
    1 Error(s)

Copy link
Member

@tmat tmat Jan 29, 2020

Choose a reason for hiding this comment

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

and replacing it with InitializeDotNetCli call (like the linux version of the script is doing)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Tried that and it also failed. At this point want to separate this out from the PR.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good.

Change Roslyn to target netcoreapp3.1 when building .NET Core assets.

Previously the code targetted a mix of netcoreapp2.1 and netcoreapp3.0.
The mix is due to default interfaces only being supported on
netcoreapp3.0 and hence our testing needed to use that. Yet at the same
time we were required to ship the compiler in SDKS that targetted
netcoreapp2.1. Now we can universally target netcoreapp3.1.
Decided to remove the property based approach to specifying a target
framework to just specifying `netcoreapp3.1` directly. The reason for
this is the following:

The advantage of the property is it makes it "easy" to change to a new
target framework in the future. That benefit is actually pretty minimal.
A simple find and replace operation is **extremely** effective in our
code base (it's less key strokes than this message). Hence the benefit
is minimal.

The downside of the property is that our code doesn't look like customer
code. Or rather it diverges from the practices that we publish. In
general I prefer to keep our code as standard as possible unless there
is a good reason to deviate. There just doesn't seem to be one here.
The TPA now considers the output directory as a trusted place for
loading assemblies. To the point it will prefer certain DLLs in the
output directory over equal ones that ship with the runtime.

This broke a number of VB tests here because we unconditionally
referenced the desktop version of Microsoft.VisualBasic. This got copied
to the output directory, included in the TPA and hence loaded during
test execution. This breaks tests because they require the .NET Core
version of Microsoft.VisualBasic.

Conditioned the reference to be desktop only so it's not present on our
.NET Core builds
Had to clean up a few nullable annotations now that we are compiling
agaist `netcoreapp3.1` and hence get the full value of the framework
annotations.

This is also problematic though because there are now two places where
the compiler can see nullable attributes that are directly used by the
developer. For example `NotNullWhenAttribute`. This is both defined in
our assemblies for non-netcoreapp target frameworks and provided by the
SDK when targeting `netcoreapp3.1`.

This causes a problem for assemblies which have the following
characteristics:

1. Target `netcoreapp3.1`
1. Reference an assembly targeting `netstandard2.0` which uses our
nullable attributes definition
1. Has IVT into (2) above

These properties essentially define all of our unit test assemblies. In
that environment it's not possible to use nullable attributes in code
because the compiler can't disambiguate which definition of
`NotNullWhenAttribute` to use. This meant I had to temporarily remove a
few attributes until we can complete dotnet#40766.
@jaredpar
Copy link
Member Author

@agocke you good here?

#if NET472
private readonly DesktopAnalyzerAssemblyLoader _loader = new DesktopAnalyzerAssemblyLoader();
#elif NETCOREAPP1_1 || NETCOREAPP2_1
#elif NETCOREAPP1_1 || 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.

wtf

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I think I can delete the NETCOREAPP1_1

@agocke
Copy link
Member

agocke commented Jan 31, 2020

Did we back away from the general "define a netcoreapp variable and use that in our #ifs"?

@sharwell
Copy link
Contributor

sharwell commented Jan 31, 2020

@jaredpar
Copy link
Member Author

The general NETCOREAPP definition is the one I am going for. I want to follow up though that I can fully nuke netcoreapp1.1 from our build tree.

@jaredpar jaredpar merged commit 0278df0 into dotnet:master Jan 31, 2020
@jaredpar jaredpar deleted the tf branch January 31, 2020 22:21
Comment on lines +348 to +350
_store[nameof(TargetType)] = value != null
? CultureInfo.InvariantCulture.TextInfo.ToLower(value)
: null;
Copy link
Contributor

Choose a reason for hiding this comment

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

💭 Is this not just value?.ToLowerInvariant()?

}

private static bool ReadBytes(BinaryReader reader, int count, [NotNullWhen(true)] out byte[]? output)
private static bool ReadBytes(BinaryReader reader, int count, out byte[]? output)
Copy link
Contributor

Choose a reason for hiding this comment

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

extern alias to the one in Microsoft.CodeAnalysis?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants