[release/5.0] Fix AssemblyInformationalAttribute version in coreclr corelib

[safern]

Fixes: #45812

I will port this back to master to have it in the same place.

The reason why this was happening is because we added IsShipping=false and that causes the InformationalVersion to include the VersionSuffix. This was intended to avoid shipping stable packages from coreclr for every servicing release which we wanted to avoid, however this was done on the wrong place as it affected System.Private.CoreLib.

Moving it to packaging.props as those properties should only affect packages.

Customer Impact

System.Runtime.InteropServices.RuntimeInformation.FrameworkDescription API returns wrong version which contains non-stable version suffix.

In summary RuntimeInformation.FrameworkDescription gives:

2.1: .NET Core 4.6.29321.03
3.1: .NET Core 3.1.10
5.0.0: .NET 5.0.0
5.0.1: .NET 5.0.1-servicing.20575.16

Testing

Manual testing. Validated that the attribute contains the right version when producing a stable release. (Note, part after + is stripped in the code)

Made sure that package versions are not affected

Risk

Low

Regression

Yes from 5.0.0, introduced when disabling stable packages for coreclr for 5.0.1

[ericstj]

@safern can we add a test for this? Seems like it might be easy to regress. Hopefully the test can correlate somehow when we're expected to be in stable mode and insist that this string is a normal version.

[ViktorHofer]

In libraries this property is set to true for anything except tests. Why is this different in coreclr? Based on the official docs, IsShipping determines if an asset is...

if it is intended to be delivered to customers via an official channel

Why do we set this to false for coreclr packages then, which are shipping to customers?

[safern]

Because we're marking them as non shipping for servicing releases since we don't want to ship them (publish to nuget). The difference with libraries, is that the libraries packages are selectively built, this are always built. I'm just moving this from one place to another to keep the fix simple.

[ViktorHofer]

That makes sense, thanks for clarifying. Do you know if we have an issue regarding building and publishing packages across the repo the same way?

[safern]

I don't think so... @Anipik do you know?

[Anipik]

no we donot

[ViktorHofer]

@Anipik Do you think it would be worth improving the current situation where packaging and publishing properties differ? If yes, could old file one?

[danmoseley]

RuntimeInformation.FrameworkDescription gives:

2.1: .NET Core 4.6.29321.03 (borked)
3.1: .NET Core 3.1.10
5.0.0: .NET 5.0.0
5.0.1: .NET 5.0.1-servicing.20575.16

[safern]

@safern can we add a test for this? Seems like it might be easy to regress. Hopefully the test can correlate somehow when we're expected to be in stable mode and insist that this string is a normal version.

I thought about it, unfortunately we can't since we run tests on CI and not on official builds. PR/CI even if the build is marked as stable, will always have a version suffix and will never be stable (they will always have -ci* suffix, logic here. We would need to run tests on official builds to be able to test this.

[ViktorHofer]

You could crack open the assembly in a target after it is built and read the AssemblyVersion :D

[joperezr]

You could crack open the assembly in a target after it is built and read the AssemblyVersion

I think what @safern was saying was that even if he does this, the -ci version would still be there. One thing we could do here would be to have this test be a msbuild-based test that runs as part of the build as opposed to a unit test, and have that msbuild-based test only run on official builds which would quickly check if the assembly version is correct which you can all do with msbuild (or worst case scenario write a small c# task that makes this check)

[ViktorHofer]

I think what @safern was saying was that even if he does this, the -ci version would still be there. One thing we could do here would be to have this test be a msbuild-based test that runs as part of the build as opposed to a unit test, and have that msbuild-based test only run on official builds which would quickly check if the assembly version is correct which you can all do with msbuild (or worst case scenario write a small c# task that makes this check)

That's what I was trying to express. Don't validate as part of a managed xunit that which we don't run as part of the official build but instead validate as part of a post-build target which would error out if the AssemblyVersion isn't the expected one. Condition that target so that it only runs when it should.

[safern]

That's a good idea. I was thinking about xunit only 😄 . Will add that.

[safern]

I added the MSBuild task and targets to validate that the attribute and version is correct.

At first I thought about just doing a simple validation of ExpectedVersion == InformationalVersion.Substring(0, IndexOf('+')) and error if that was false in a simple MSBuild target, however, even if InformationalVersion property is correct, the attribute could be wrong, since a public API relies on the attribute being correct I decided to add a task that cracked open the assembly and inspected the attribute. Please let me know if you think it is overkill to add a task and I can revert it and do it in the simple MSBuild way that doesn't crack open the attribute.

Second, I started using System.Reflection.MetadataLoadContext, however, since this is an MSBuild task, I was getting:

C:\repos\runtime\Directory.Build.targets(43,5): error MSB4018: The "ValidateAssemblyInformationalVersion" task failed unexpectedly. [C:\repos\runtime\src\coreclr\src\System.Private.CoreLib\System.Private.CoreLib.csproj]
C:\repos\runtime\Directory.Build.targets(43,5): error MSB4018: System.IO.FileNotFoundException: Could not load file or assembly 'System.Reflection.MetadataLoadContext, Version=5.0.0.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51'. The system cannot find the file specified. [C:\repos\runtime\src\coreclr\src\System.Private.CoreLib\System.Private.CoreLib.csproj]
C:\repos\runtime\Directory.Build.targets(43,5): error MSB4018: File name: 'System.Reflection.MetadataLoadContext, Version=5.0.0.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51' [C:\repos\runtime\src\coreclr\src\System.Private.CoreLib\System.Private.CoreLib.csproj]
C:\repos\runtime\Directory.Build.targets(43,5): error MSB4018:    at Microsoft.DotNet.Build.Tasks.ValidateAssemblyInformationalVersion.Execute() [C:\repos\runtime\src\coreclr\src\System.Private.CoreLib\System.Private.CoreLib.csproj]
C:\repos\runtime\Directory.Build.targets(43,5): error MSB4018:    at Microsoft.Build.BackEnd.TaskExecutionHost.Microsoft.Build.BackEnd.ITaskExecutionHost.Execute() [C:\repos\runtime\src\coreclr\src\System.Private.CoreLib\System.Private.CoreLib.csproj]
C:\repos\runtime\Directory.Build.targets(43,5): error MSB4018:    at Microsoft.Build.BackEnd.TaskBuilder.ExecuteInstantiatedTask(ITaskExecutionHost taskExecutionHost, TaskLoggingContext taskLoggingContext, TaskHost taskHost, ItemBucket bucket, TaskExecutionMode howToExecuteTask) [C:\repos\runtime\src\coreclr\src\System.Private.CoreLib\System.Private.CoreLib.csproj]

The only way to do that was to publish instead of build the installer tasks, and that would've been more complicated since you need to specify the core library path and assembly search paths, so if we want to enable this on other projects in the future it would involve more logic. So I decided to go with MetadataReader since you don't need to load any dependencies and you can just get the info needed from there.

Let me know if you have any objections or suggestions.

I validated by removing my fix and I got an error as expected:

C:\repos\runtime\Directory.Build.targets(35,5): error : AssemblyInformationalVersion 5.0.1-dev.20609.11 doesn't match expected version 5.0.1 in: C:\repos\runtime\artifacts\bin\coreclr\Windows_NT.x64.Release\IL\System.Private.CoreLib.dll [C:\repos\runtime\src\coreclr\src\System.Private.CoreLib\System.Private.CoreLib.csproj]

[Anipik]

@safern can we merge this one if the only thing remaining is tests ?

[safern]

I chatted with @ericstj and we decided to keep the change simple and think more about a better way to test it rather than duplicating the logic to calculate the expected value for the version. I will add the test in master and then we can backport the test to 5.0, but let's get this in to fix the issue on 5.0.2.