-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Launch Crossgen2 via explicitly specified .NET host #15529
Conversation
src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Publish.targets
Outdated
Show resolved
Hide resolved
Crossgen2 is published as a self-contained app. Why do we want to launch it using a host? Is that to support scenarios when crossgen2 is not self-contained? |
I believe I have addressed the gist of David's feedback. For Anton's suggestion, as discussed offline this is non-trivial to do as publishing of the "self-contained Crossgen2 app" by itself needs the crossgenned framework as a prerequisite. Having said that, I don't think we can afford to wait until this fix propagates to the runtime repo by means of rolling forward the SDK version. The SDK upgrade is scheduled to be merged in as part of the March infra roll-out, please see so that one more upgrade (to include changes in this PR) will likely happen in April if not later and that's far too late for our other CG2 efforts. Apart from finalizing this change I plan to try to address or mitigate reviewer feedback on my other PR so that we can start testing CG2-compiled SDK ASAP. One way or another, please take another look at this change when you have a chance. Thanks Tomas |
@trylek, My concerns have been addressed, but I'd rather somebody else walk through and verify that the behavior is correct as I'm generally not familiar with this codebase. I'd like to see @AntonLapounov provide a review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think all error messages should go to resource files to allow localization.
6aec4a7
to
936b4c5
Compare
src/Tasks/Microsoft.NET.Build.Tasks/PrepareForReadyToRunCompilation.cs
Outdated
Show resolved
Hide resolved
I have noticed that EmitSymbols is not handled consistently - PrepareReadyToRunCompilation was setting it as metadata on the compilation item whereas RunReadyToRunCompiler expected it as a task parameter. I ended up converting RunReadyToRunCompiler to use the compilation entry metadata as the logic to set the flag in PrepareReadyToRunCompilation is non-trivial and trying to duplicate it in the Microsoft.NET.Publish.targets script would be ugly and error prone. Thanks Tomas
The logic for emitting --pdb-path and --perfmap-path arguments was incorrect as Crossgen2, just like Crossgen, only expects the directory name, not the complete output file name for PDB / PerfMap generation. I have already verified locally this on Windows by injecting the fresh-built SDK into my runtime clone with the CG2 publishing change, I'm now working on doing the same on Linux. Thanks Tomas
@AntonLapounov - sigh, apparently I need to add some provisions to PrepareReadyToRunCompilation to cater for the fact that Crossgen2 until now hasn't been generating the MVID into the PerfMap name. I'll send out a PR for fixing that but that will only make it to Preview 2 - am I right to presume that we need something like a minor version check in the code? |
I don't think you need a minor version check. Make the fix in Runtime repo and wait a bit until it is picked up by the SDK repo. SDK will use the latest Crossgen2 package. |
Well, if I don't merge the SDK change in this week, it's going to slip to Preview 3 and its back-propagation to runtime will get delayed until Preview 3 ships (becomes generally available); I'm worried that pushes us uncomfortably close to the general .NET 6 shipping date. |
@trylek You may temporarily disable emitting perf maps in R2R SDK tests for .NET 6 to unblock this PR. Do I understand correctly that you want to place the MVID in the file name for non-composite mode only? Is having MVID in the file name important? Below are the errors I see in the log:
|
@trylek Is that a problem that for the composite perf map you are not using any MVID? If not, maybe omitting MVID in non-composite mode would be OK as well. |
Right, the MVID is used to make sure that we only use a "matching" symbol file. Sounds like for composite mode, we'd need to do something else, but perhaps we'd just depend on a recent enough version of the perf tools to be available on the box, and then it shouldn't matter, since we'd use the built-in Linux perf tool's jitdump functionality. |
/cc @dotnet/crossgen-contrib
Figuring out right now how to inject this into the runtime repo in order to actually test it, sending out in hope of early PR feedback.
Thanks
Tomas