Add aot.slnx for working on crossgen2 and ilc at the same time - #133764
Conversation
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
Tagging subscribers to this area: @agocke, @dotnet/ilc-contrib |
There was a problem hiding this comment.
🟡 Changes recommended
The combined solution omits required ILC projects, and the documented workflow does not mention it.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds aot.slnx, a combined solution for working on ILC and crossgen2 projects.
Changes:
- Adds shared project configurations and platform mappings.
- Includes ILC, crossgen2, ReadyToRun, and ILLink-related projects.
Review findings:
- Moderate (2 votes): Add the linker shared/test projects and existing Checked/Debug mappings omitted from
ilc.slnx. - Nit (1 vote): Update NativeAOT documentation to mention
aot.slnx.
File summaries
| File | Description |
|---|---|
src/coreclr/tools/aot/aot.slnx |
Defines the combined AOT solution and project mappings. |
Review details
Suppressed comments (1)
src/coreclr/tools/aot/aot.slnx:1
- The NativeAOT build documentation still directs compiler work to
ilc.slnxand lists only the existing compiler/runtime solutions (docs/workflow/building/coreclr/nativeaot.md:63-67), so this new combined solution is not discoverable through the documented workflow. Please update that solution list and the nearby workflow instructions to mentionaot.slnx.
<Solution>
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Lite
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
Configuration mappings need reconciliation, and the workflow documentation should be updated.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (2)
src/coreclr/tools/aot/aot.slnx:65
- These mappings are inherited from
ilc.slnx, but they conflict with the crossgen2 project mapping:crossgen2.slnxmapsILCompiler.TypeSystemtox64for*|Any CPUand leaves it buildable (crossgen2.slnx:52-56), whereas this block mapsAny CPUtox86and setsBuild=false. Consequently, selecting Any CPU in the combined solution changes both whether TypeSystem is a selected build and which project platform is passed compared with the existing crossgen2 solution, which can make crossgen2 debugging/build output diverge. Please reconcile this shared-project mapping with the crossgen2 configuration or explicitly document the intended divergence.
[!NOTE] This review comment was generated by GitHub Copilot.
<Project Path="ILCompiler.TypeSystem/ILCompiler.TypeSystem.csproj">
<Platform Solution="*|Any CPU" Project="x86" />
<Platform Solution="*|x64" Project="x64" />
<Platform Solution="*|x86" Project="x86" />
<Build Solution="*|Any CPU" Project="false" />
src/coreclr/tools/aot/aot.slnx:32
- This shared
ILCompiler.Diagnosticsentry does not preserve the established crossgen2 mapping. Incrossgen2.slnx,Checked|*is mapped to projectDebug(andR2RDump.slnxdoes the same), but this block letsCheckedflow through. That is not a cosmetic difference:src/coreclr/tools/Directory.Build.propsgivesCheckedits own optimized/DEBUG semantics, soChecked|x64inaot.slnxproduces a different diagnostics configuration fromcrossgen2.slnx. Please preserve the established mapping or decide and document a single merged policy for this shared project.
[!NOTE] This review comment was generated by GitHub Copilot.
<Project Path="ILCompiler.Diagnostics/ILCompiler.Diagnostics.csproj">
<Platform Solution="*|Any CPU" Project="x86" />
<Platform Solution="*|x64" Project="x64" />
<Platform Solution="*|x86" Project="x86" />
<Build Solution="*|Any CPU" Project="false" />
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Lite
|
Can we add ilverification, Ilverify, and iltrim as well? They all share the managed type system code too |
If we want to cover "everything using managed type system", we'd also need to include System.Private.TypeLoader, dotnet-pgo, r2rdump, etc and at some point it becomes pointless. |
|
I mainly mention ILVerification as it's the other place I've been burned by compilation errors that don't show up when my solution has been loaded (as compared to the other tools where that generally hasn't occurred). dotnet-pgo had hit that once or twice, but not nearly as much as ILVerification has for me. |
I looked at this as a temporary crutch for WASM bring up. I expect the extra projects to have impact on solution load and project system responsiveness to the extent that aot.slnx would only be used by people who expect ilc/crossgen interactions, like the WASM bringup work. Not for normal daily work. I don't mind either way. |
There was a problem hiding this comment.
🔵 Needs a closer look
The missing configuration mapping should be restored before approval.
Review details
Suppressed comments (1)
src/coreclr/tools/aot/aot.slnx:32
- The existing
crossgen2.slnxexplicitly maps every Checked solution configuration forILCompiler.Diagnosticsto the Debug project configuration. This combined solution omits that override, so selecting Checked here builds this dependency as Checked instead and no longer preserves the crossgen2 solution's known configuration mapping. Please carry over the same Checked-to-Debug mapping.
<Project Path="ILCompiler.Diagnostics/ILCompiler.Diagnostics.csproj">
<Platform Solution="*|Any CPU" Project="x86" />
<Platform Solution="*|x64" Project="x64" />
<Platform Solution="*|x86" Project="x86" />
<Build Solution="*|Any CPU" Project="false" />
- Files reviewed: 1/1 changed files
- Comments generated: 0 new
- Review effort level: Lite
|
/ba-g known failure |
…t#133764) Add a new solution file with the superset of ilc.slnx and crossgen2.slnx for cross-cutting work.
It would be nice to not have to switch between solutions when working on cross-cutting changes.