-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Move tools to use LKG ILC #107772
base: main
Are you sure you want to change the base?
Move tools to use LKG ILC #107772
Conversation
Are we going to build the non-NativeAOT crossgen2 (the singlefilehost version) against the LKG bits? If so, could we instead just update/use the crossgen2.csproj project and have 2 projects instead of continuing to have the 3 separate projects? |
By that you mean, are we going to pull the single-file host from the LKG as well? I hadn't thought of that, but it makes sense. |
Yep that's what I was thinking of. I think however we ship crossgen2, we should ship it on equivalent runtime bits. The alternative would likely be more expensive in infra maintenence for minimal gain, unless it's required by source-build or something. |
@@ -1,14 +1,7 @@ | |||
<Project Sdk="Microsoft.NET.Sdk"> | |||
<PropertyGroup> | |||
<CrossHostArch Condition="'$(CrossBuild)' == 'true' or '$(TargetArchitecture)' != '$(BuildArchitecture)' or '$(HostOS)' != '$(TargetOS)' or '$(EnableNativeSanitizers)' != ''">$(BuildArchitecture)</CrossHostArch> |
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.
For ilc, this is set unconditionally in the crossarch project
<OutputPath>$(RuntimeBinDir)/$(CrossHostArch)/ilc/</OutputPath> |
Also, we should make the crossgen and ilc project files use the same naming scheme. I like the naming scheme for ilc project files (ILCompiler.csproj, ILCompiler_crossarch.csproj).
I've been experimenting locally, and I don't think we should get rid of the publish project either. The problem is that the SDK is architected such that I can't think of a way to do that without a different project if we also want things to use the non-published copy. If I set |
ilc has two projects (native and cross-arch). It does not have any special handling of publishing. Can crossgen be on the same plan? |
I think ILC is the incorrect one here -- while not setting Thus far ILC has not depended on any of those behaviors, but I think that's accidental. |
Maybe I am missing the bigger picture. Coverage-wise, both approaches are eventually testing what is being shipped:
Live build is better IMO, and it gives us control over enabling nativeaot for new platforms without waiting for the entire update cycle, runtime->sdk->runtime (which takes a month or two). End to end tests in dotnet/sdk repo are covering the nuget feed scenario. |
But it's also testing every version, as you're developing it on a local machine and in CI. So in addition to the complexity in the build system needed to produce the right phase ordering, you also make local development and CI uniquely unstable by changing Native AOT. Add to all of this, debug builds become unusably slow because the debug JIT is incredibly slow. Optimizing for live builds is optimizing for release validation, while using LKG builds is optimizing for infra simplicity and developer experience. |
Yup, @jkotas explained it to me about the release vs. debug performance penalty and I agree that live build (for non-testing) should be on release plan. BTW, are you looking to incorporrate the scenario #105004 is covering? I can close that one. Basically it's this workflow https://github.com/am11/CrossRepoCITesting/actions/runs/10847119420/workflow (also covered in runtime CI, see eng/pipelines changes in the PR). |
In theory, yes. In practice, there isn't an LKG apphost for FreeBSD right? I haven't thought of what we should do in that case. Perhaps the answer is "require an LKG apphost just like we require an LKG runtime" |
I think we should introduce a new build option that builds everything that comes from LKG (including LKG apphost if necessary) and require invoking build with that option before invoking regular build if there is no LKG available for the target platform. #105004 (comment) has details. Introducing this build option should be a separate PR from this one. |
Yes, it's about the |
Starts with crossgen2 and moves shared helpers into a central targets file.
src/coreclr/crossgen-corelib.proj
Outdated
@@ -5,7 +5,8 @@ | |||
</PropertyGroup> | |||
|
|||
<ItemGroup> | |||
<ProjectReference Include="$(CoreClrProjectRoot)/tools/aot/crossgen2/crossgen2_inbuild.csproj" OutputItemType="Crossgen2" /> | |||
<ProjectReference Condition="'$(UseCrossArchCrossgen2)' != 'true'" Include="$(CoreClrProjectRoot)/tools/aot/crossgen2/crossgen2_publish.csproj" ReferenceOutputAssembly="false" /> | |||
<ProjectReference Condition="'$(UseCrossArchCrossgen2)' == 'true'" Include="$(CoreClrProjectRoot)/tools/aot/crossgen2/crossgen2_crossarch.csproj" /> |
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.
Should we get rid of the UseCrossArchCrossgen2
property, and build and use the cross-arch variant unconditionally? Most of the crossgen2 code is in libraries, building the small .exe twice should not be a big deal. It would save us from issues that are specific to non-cross vs. cross builds.
If you adopt this suggestion, I think the original "inbuild" suffix would be more appropriate.
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.
We could -- the benefit would be that the AOT crossgen is much faster than coreclr one, so our build actually gets faster if we use the ILC copy.
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.
Eh, it's a few minutes, doesn't seem worth the complexity.
eng/Subsets.props
Outdated
<ProjectToBuild Include="$(CoreClrProjectRoot)tools\aot\crossgen2\crossgen2_inbuild.csproj" Category="clr" /> | ||
<!-- skip the architectures that don't support self-contained at all --> | ||
<ProjectToBuild Include="$(CoreClrProjectRoot)tools\aot\crossgen2\crossgen2_publish.csproj" Condition="'$(TargetArchitecture)' != 'armel' and '$(TargetArchitecture)' != 'riscv64'" Category="clr" /> |
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.
The comment is a bit misleading. Here the problem is not about target not supporting the self-contained, but rather we don't have community-supported platforms' SDK and LKG Microsoft.NETCore.App.Runtime.linux-{platform}
in our CI, i.e. riscv64
, loongarch64
, freebsd-{x64,arm64}
, illumos-x64
etc.
Also, armel should use arm (via ToolsRID
) per
"arm" or "armel" => TargetArchitecture.ARM, |
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.
Since this is going to break other community platforms, maybe we should either exclude all community platforms (Condition=<all official platforms>
) or keep them using the live build.
Alternatively, we can simplify the relevant parts of the infrastructure and allow community platforms to provide their hook into the install script (dotnet/install-scripts#501). This way, we won't need to keep track of the distinction between community and official versions throughout the process. For example, switching to the LKG version will be straightforward, with no exceptions; since any "bring-your-own-platform-SDK" issues would have been resolved earlier in the infra bring-up work for that platform.
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 this should be solved by the proposed two-phase build, but I'm not quite sure of the set up right now.
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.
Honestly, the easiest way to fix this is to just require that community builds publish a runtime pack in their LKG toolset. If their local SDK contains a freebsd-x64
runtime pack, it will be used during the build.
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.
Yes, something like that.
FreeBSD folks already have their own nuget feed https://github.com/Thefrank/dotnet-freebsd-crossbuild?tab=readme-ov-file#how-do-i-use-the-output. We just need to make a templated / example maybe using https://docs.github.com/en/packages/working-with-a-github-packages-registry/working-with-the-nuget-registry, and then integrate it with source-build. The first round of port is bit patchy (check the current haiku or old illumos PRs), after that round, things get pretty smooth. We can let them provide their nuget feed and where to download the sdk tar.gz via install script.
My point was that this distinction of "official" vs. "community" is not necessary to sprinkle around the infra if we think about it. It is just an entry in pipeline matrix for machine building the signed release and a note in doc.
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.
Honestly, the easiest way to fix this is to just require that community builds publish a runtime pack in their LKG toolset
If the CI builds for community targets were to take dependency on this, new LKG packages for community targets would have to be published in lock step with the Microsoft published ones. I do not think we want to be dealing with this moving part during LKG updates.
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 there are options like this:
- we don't validate their build in our CI, we can let them create with same OfficialBulidId used in LKG. They are already using that: https://github.com/Thefrank/dotnet-freebsd-crossbuild/blob/13d2f5a393d64e377be96ff841a8045d66d1f80c/build.sh#L63
- we keep validating their build, and in LKG update PR, we use a command like
@dotnet-community-bot build LKG <version>
and it will trigger the same job which publishes for a given OfficialBuildId.
I believe this is ready for review. Two things this doesn't handle:
I think both of those are best left up to later PRs. |
eng/Subsets.props
Outdated
<ProjectToBuild Include="$(CoreClrProjectRoot)tools\aot\crossgen2\crossgen2_inbuild.csproj" Category="clr" /> | ||
<!-- skip the architectures that don't have LKG runtime packs --> | ||
<ProjectToBuild Include="$(CoreClrProjectRoot)tools\aot\crossgen2\crossgen2_publish.csproj" Condition="'$(NativeAotSupported)' == 'true'" Category="clr" /> |
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.
A few questions:
- Was there a reason it was moved below?
- This project is also built by
src/installer/pkg/sfx/Microsoft.NETCore.App/Microsoft.NETCore.App.Crossgen2.sfxproj
(i.e.packs
subset), do we need this condition there? - If answer to 2. is "yes", then this is a regression on linux-riscv64 and illumos-x64 etc. where
NativeAotSupported
is false; i.e. main publishes crossgen2, while PR doesn't.. until the "later PR" is on the horizon, right?
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.
(1) No, just accidental due to moving things around
(2) I don't think it needs to be
@@ -9,6 +9,7 @@ | |||
Condition="'$(DisableVersionCheckImported)' != 'true'" /> | |||
|
|||
<Import Project="$(RepoRoot)eng/liveBuilds.targets" /> | |||
<Import Project="$(RepositoryEngineeringDir)toolAot.targets" /> |
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.
Do we still need to build crossgen during test build? Would it make more sense to go back to using the one created during the build?
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.
Looks like there are still reasons to keep this. In particular, I moved all the package overrides here, so if we need to use new ILC packages the overrides are here.
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 am talking about https://github.com/dotnet/runtime/blob/main/src/tests/Common/Directory.Build.targets#L66-L78 and UsePublishedCrossgen2
property. Building tests is reaching out into the product build to publish crossgen2. It should not be needed anymore since the ordering problems that this was working around should not exist after this PR. We should be able to use what we have produced during the product build instead. (My assumption was that the line that I have commented on is necessary to make this work.)
I agree that ability to override ILC LKG as necessary is good. But building tests should not need ILC LKG to publish. If tests need to publish, they should use the live ILC.
Will community supported platforms, in my case FreeBSD, still be able to generate a working runtime/ILC/Crossgen when building under Linux? |
@Thefrank I don't think there's anything preventing it, but the packages will not be built by default |
ugh, turns out we still need the inbuild ILC for the tests |
I think this is ready to merge |
Starts with crossgen2 and moves shared helpers into a central targets file.