Fix crossgenning of framework libraries on Windows #747
Conversation
src/coreclr/build-test.cmd
Outdated
| REM don't precompile anything from CoreCLR | ||
| if /I exist %CORE_ROOT_STAGE%\%2 exit /b 0 | ||
| if /I exist %CORE_ROOT_STAGE%\%AssemblyName% exit /b 0 |
There was a problem hiding this comment.
Does this mean that we won't be crossgening System.Private.CoreLib.dll with crossgen2?
There was a problem hiding this comment.
It probably does and I concur it's something worth discussing moving forward just like the default compilation of S.P.C with legacy Crossgen within build.cmd/sh.
There was a problem hiding this comment.
I think this is a bit different from using the new crossgen in build.cmd/sh. When we run the build-tests.cmd with the option asking for crossgen2 compilation, is it strange to get the whole framework crossgened with crossgen2 except for the S.P.C. I wonder if we could just add a check here that if we are using crossgen2, we would precompile the S.P.C. too.
There was a problem hiding this comment.
I agree it's weird. I basically left that in as a pre-existing conditional statement but thinking about this some more, what about throwing this away altogether? It doesn't make too much sense to me in the "new world".
trylek
changed the title
| set /a __exitCode = !errorlevel! | ||
| ) | ||
|
|
||
| echo %__CrossgenCmd% |
There was a problem hiding this comment.
It seems this should be replaced by the similar code from the else branch above and deleted from the else branch.
There was a problem hiding this comment.
Fixed, thanks for pointing that out!
src/coreclr/build-test.cmd
Outdated
| ) else ( | ||
| "!CORE_ROOT_STAGE!\crossgen2\crossgen2 -r:"!CORE_ROOT!\*.dll" -O --inputbubble -out:"__CrossgenOutputFile" "%1" >nul 2>nul | ||
| set __CrossgenCmd=!__CrossgenExe! -r:"!CORE_ROOT!\*.dll" -O --inputbubble --out:!__CrossgenOutputFile! !AssemblyPath! >nul 2>nul |
There was a problem hiding this comment.
I think that to eliminate all the ugly warnings on bad image format of files that are .dll but not framework assemblies, we could replace -r:"!CORE_ROOT!\*.dll by -r:"!CORE_ROOT!\System.*.dll -r:"!CORE_ROOT!\Windows.*.dll I think this covers all the framework assemblies.
There was a problem hiding this comment.
Oh, and -r:!CORE_ROOT!\mscorlib.dll too, some tests need that.
There was a problem hiding this comment.
Good suggestion, thanks, fixed!
src/coreclr/build-test.cmd
Outdated
| ) else ( | ||
| "!CORE_ROOT_STAGE!\crossgen2\crossgen2 -r:"!CORE_ROOT!\*.dll" -O --inputbubble -out:"__CrossgenOutputFile" "%1" >nul 2>nul | ||
| set __CrossgenCmd=!__CrossgenExe! -r:"!CORE_ROOT!\*.dll" -O --inputbubble --out:!__CrossgenOutputFile! !AssemblyPath! >nul 2>nul |
There was a problem hiding this comment.
I suppose the >nul 2>nul was supposed to be part of the __CrossgenCmd, but it is applied to the set command instead. It should be placed on the !__CrossgenCmd! line if we want to suppress the crossgen output.
There was a problem hiding this comment.
Deleted per our offline chat, there's no sense in throwing away diagnostic information (and there's not a ton of it).
trylek
force-pushed
the
CrossgenFrameworkFix
branch
from
505d75f
to
7a1f5f3
Compare
I tried to slightly improve the data flow by creating explicit environment variables for the two arguments. There were a bunch of errors regarding % vs !, duplicate double-quoting and the like. It seems to be working now. Thanks Tomas
1) The ordering of steps in build-test was incorrect - we need to patch CORE_ROOT with live-live libraries build before Crossgenning the framework therein, otherwise weird things happen (and the result is incorrect in any case). 2) Removed misplaced >nul 2>nul - we agreed with JanV that it's better to not throw diagnostic information away. 3) Applied JanV's suggestion for improvement - using a tighter filter for reference assemblies - Microsoft.*.dll / System.*.dll / mscorlib.dll instead of just *.dll that ended up picking tons of garbage like clrjit.dll and the various Win32 API contracts, both in the build-test script and in CLRTest.Crossgen.targets. Thanks Tomas
trylek
force-pushed
the
CrossgenFrameworkFix
branch
from
2865a73
to
9da22d4
Compare
ghost
locked as resolved and limited conversation to collaborators
I tried to slightly improve the data flow by creating explicit
environment variables for the two arguments. There were a bunch
of errors regarding % vs !, duplicate double-quoting and the like.
It seems to be working now.
Thanks
Tomas