Switch over framework packaging to use Crossgen2

[trylek]

Based on Jeremy's advice I have modified the pack build projects
to use the Crossgen2 compiler. As we're still using the 5.0 version
of the SDK, this change synthesizes the name of the JIT library -
upgrading SDK to version 6 will facilitate further cleanup in
these scripts.

Thanks

Tomas

P.S. I'm not sure whether we should add crossgen2 to 'RuntimeFiles' - I originally did that but the packs build complains the "public key not found" in the managed crossgen2 assemblies. I would naively assume that we're publishing Crossgen2 as a separate nuget package so we shouldn't need to include in runtime too.

/cc @dotnet/crossgen-contrib

[trylek]

Hmm, so on Linux we apparently cannot run the "exe" version of Crossgen2 because it fails to locate the framework. @vitek-karas - is there any magic environment variable we could use to present the Crossgen2 app with the dotnet host installed by Arcade in the repo root? Otherwise we'll need to switch over to using the dotnet launcher and that will require changes even in the 6.0 SDK as its current main branch doesn't have that support.

[vitek-karas]

Set DOTNET_ROOT to point to the root of the "install". In case of repo local it typically looks something like /path/to/repo/.dotnet.

[jkotas]

I think you want to use DOTNET_ROOT on Windows too. It works on Windows because of it picks up a random version of dotnet that comes with Visual Studio. We try to always use our own local version of dotnet.

[trylek]

I believe I have addressed @jkotas' and @jkoritzinsky's PR feedback and the installer tests are now passing. I'm however seeing a few library test failures, most notably System.Linq.Expressions.Tests.ConvertTests.ConvertFloatToUIntTest and System.Linq.Expressions.Tests.ConvertTests.ConvertFloatToNullableUIntTest, these repro on several platforms and indicate a potentially systemic failure. I plan to look deeper into these tests next unless someone has another explanation for the failures.

[trylek]

In fact I have a hard time to imagine how my "packs.product"-only changes could affect library tests but it's been a while since I was buried full time in runtime repo consolidation and things may have changed since then.

[trylek]

/cc @dotnet/runtime-infrastructure

[jkoritzinsky]

I'm however seeing a few library test failures, most notably System.Linq.Expressions.Tests.ConvertTests.ConvertFloatToUIntTest and System.Linq.Expressions.Tests.ConvertTests.ConvertFloatToNullableUIntTest, these repro on several platforms and indicate a potentially systemic failure. I plan to look deeper into these tests next unless someone has another explanation for the failures.

Those failures are known failures.

[ViktorHofer]

nit:

Suggested change

	<CrossDir></CrossDir>
	<CrossDir />

[trylek]

Oh, sure, good point, thanks for spotting.

[trylek]

Fixed in 2nd commit (in ReadyToRun.targets where I moved the logic that was originally duplicated between the two sfxproj scripts).

[ViktorHofer]

Looks like these properties are a duplicate of the ones in the touched file above. Would it make sense to extract them into a props file and import them in both projects to avoid the duplication?

[trylek]

I agree that's a straightforward copy but I suspect it can't be props as it's a target with a state. I think we could make it an Import but frankly speaking 90% of the code will be deleted after Crossgen1 and SDK 5.0 are retired so I'm not sure if it's worth the effort, ultimately the remaining Crossgen2 variant should boil down to less than half dozen lines of msbuild script code.

[ViktorHofer]

If this is just temporary then I'm fine with the duplication

[trylek]

Removed duplication with #48201.

[ViktorHofer]

Why do you need DOTNET_ROOT to be set? We didn't need this before except for Visual Studio and ILLink (?)?

[trylek]

On Linux, Crossgen2 executable doesn't launch without DOTNET_ROOT being set (please see discussion above in this thread). I guess the clean solution would be to use the dotnet launcher but that's something yet to be implemented in the sdk repo so even after its upgrade to 6.0 it won't come for free.

[ViktorHofer]

Can we set this environment variable only in the place where crossgen2 is actually invoked? Is that happening in the SDK? Is there a hook to pass environment variable to that invocation directly to avoid global environment variables being set? An MSBuild Exec Task allows to pass in env vars.

[ViktorHofer]

We don't want to set this environment variable in the root build script as it means that we depend on it throughout the whole build which we actually don't as only crossgen2 needs it. Setting it in the place where crossgen2 is invoked guarantees, that the sfxprojs are buildable when that env var isn't set (i.e. when invoked directly with dotnet build, instead of from the root build.cmd/sh).

[trylek]

Well, I guess it's mostly a sequencing problem. My initial goal was to just make this work somehow; with help from many people I've managed to achieve that in the current change and we can now discuss the logistics of getting that in. I believe that the problem regarding DOTNET_ROOT is basically a deficiency of the RunReadyToRunCompiler task that should accept a parameter specifying the path to .NET CLI and use the "dotnet" launcher to launch Crossgen2. This logic is absent both in SDK 5.0 and in current main branch of the repo i.o.w. it's not available to us now and won't become immediately available even after we upgrade the SDK repo drop. I can easily sit on this change for a week or two and make the prerequisite changes first in other repos but we need to coordinate the integrations to make sense.

[trylek]

In case this is a too oblique answer to your question, the ToolTask base class of RunReadyToRunCompiler, please see

https://github.com/dotnet/sdk/blob/810129ba5b35dbf77f8d6d84171eb7363693551d/src/Tasks/Microsoft.NET.Build.Tasks/RunReadyToRunCompiler.cs#L14

currently doesn't support injecting environment variable overrides into the child process. It is definitely doable, we're doing such a thing in R2RTest and in other places, it's just not there yet. This gets us back to the sequencing thing, it seems to me we're converging towards the conclusion that the best way forward is to fix this cleanly in the SDK repo and simplify this change to just consume it, in such case we'd no longer require global environment overrides like DOTNET_ROOT.

[trylek]

Actually the formulation I used is somewhat imprecise - I don't claim ToolTask doesn't support setting environment variables, I have no idea if it does and maybe / likely it does, it's just not used / implemented in the RunReadyToRunCompiler task class derived from ToolTask and so implementing it would require a code change. And my current thinking is that, if we're about to do a code change to the RunReadyToRunCompiler task, it would be easiest to cleanly switch it over to running Crossgen2 using the dotnet launcher and avoid tampering with environment variables.

[ViktorHofer]

in response to your ToolTask statement, ToolTask already provides a way to specify env vars: https://docs.microsoft.com/en-us/dotnet/api/microsoft.build.utilities.tooltask.environmentvariables?view=msbuild-16-netcore#Microsoft_Build_Utilities_ToolTask_EnvironmentVariables. It's probably just a matter of adding an environment variables property to the RunReadyToRunCompiler task invocation.

Regarding the logistics, I agree that this should be cleanly fixed by the SDK but that doesn't mean that the required environment variable can't be set just in time before the RunReadyToRunCompiler task is invoked.

I was thinking about adding a target that hooks onto _CreateR2RImages as a BeforeTargets and sets the environment variable.

[trylek]

Good point, fixed.

[trylek]

OK, so this is my roster of items to follow up for now:

1. SDK repo drop in the runtime repo needs upgrading to 6.0 (will remove the need to synthesize JitPath in the scripts);
2. SDK repo implementation of RunReadyToRunCompiler lacks actionable telemetry, cf. RunReadyToRunCompiler task doesn't produce actionable information on failures sdk#15466
3. Microsoft.NETCore.App.Crossgen2.sfxproj and Microsoft.NETCore.App.Runtime.sfxproj contain a duplicative section related to R2R compilation (Crossgen / Crossgen2 configuration); most of this should go away once SDK 5.0 and Crossgen1 are retired.

Please remind me if I missed something.

[trylek]

It looks like I managed to pass installer builds using the modification per Viktor's suggestion. @jkoritzinsky / @ViktorHofer, can you please take another look when you have a chance?

Thanks

Tomas

[hoyosjs]

The only problem with this is it will eventually change this property, some time after _CreateR2RImages ran. The only 2 ways I know for somewhat strict ordering are using a dependent target (the you don't own case, but there's an extensibility model, and it's cleaner), or the CallTarget route (which has few good uses, and unless we own _CreateR2RImages, it won't be a good idea..

[trylek]

The exact problem here is that, while we "own" _CreateR2RImages, we do so in a different repo (sdk) and the entire trick here is about not having to wait for its propagation to the runtime repo as that will take about two months or more. If I could merge in my SDK PR today and propagate it to runtime tomorrow, this would be a no-concern.

[ViktorHofer]

LGTM if this works as expected. We will upgrade the SDK in less than two weeks anyway so I'm not too concerned about the hacks. And after that we will update the SDK probably on a monthly cadence.

[AntonLapounov]

Apparently Crossgen2 is unable to emit PDBs on ARM64. We would need to grab Microsoft.DiaSymReader.Native.arm64.dll for that.

C:\Runtime\dotnet.cmd C:\Runtime\artifacts\bin\coreclr\windows.arm64.Release\crossgen2\crossgen2.dll -o:C:\Runtime\artifacts\bin\coreclr\windows.arm64.Release\System.Private.CoreLib.dll -r:C:\Runtime\artifacts\bin\coreclr\windows.arm64.Release\IL\*.dll --targetarch:arm64 -O C:\Runtime\artifacts\bin\coreclr\windows.arm64.Release\IL\System.Private.CoreLib.dll --pdb --pdb-path:C:\Runtime\artifacts\bin\coreclr\windows.arm64.Release\PDB
  C:\Runtime\.dotnet
  Emitting R2R PE file: C:\Runtime\artifacts\bin\coreclr\windows.arm64.Release\System.Private.CoreLib.dll
  Emitting PDB file: C:\Runtime\artifacts\bin\coreclr\windows.arm64.Release\PDB\System.Private.CoreLib.ni.pdb
EXEC : error : An attempt was made to load a program with an incorrect format. (0x8007000B) [C:\Runtime\src\coreclr\crossgen-corelib.proj]
  System.BadImageFormatException: An attempt was made to load a program with an incorrect format. (0x8007000B)
     at ILCompiler.Diagnostics.PdbWriter.CreateNGenPdbWriter64(String ngenImagePath, String pdbPath, Object& ngenPdbWriter)