Skip to content
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

Use live apphost when publishing ilc as singlefile #105004

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

am11
Copy link
Member

@am11 am11 commented Jul 17, 2024

Using the following command to cross-build product for FreeBSD:

docker run --rm -v$(pwd)/runtime:/runtime -e ROOTFS_DIR=/crossrootfs/x64 \
  mcr.microsoft.com/dotnet-buildtools/prereqs:cbl-mariner-2.0-cross-amd64-freebsd-14 \
  sh -c '/runtime/build.sh clr+libs+packs -c Debug -os freebsd -cross &&
  tdnf install -y file unzip && cd /tmp &&
  unzip /runtime/artifacts/packages/Debug/Shipping/runtime.freebsd-x64.Microsoft.DotNet.ILCompiler.9.0.0-dev.nupkg &&
  find . -name ilc -exec file {} \;'

before:

./tools/ilc: ELF 64-bit LSB pie executable, x86-64, version 1 (SYSV), dynamically linked, interpreter /lib64/ld-linux-x86-64.so.2, for GNU/Linux 2.6.32, BuildID[sha1]=771fdbe539f221e5b1e4e6dc000846f10d503b3f, stripped

after:

./tools/ilc: ELF 64-bit LSB pie executable, x86-64, version 1 (FreeBSD), dynamically linked, interpreter /libexec/ld-elf.so.1, for FreeBSD 14.0 (1400097), FreeBSD-style, BuildID[sha1]=4b265d82eb3e1de5315251f7a5db2713ced94d2e, stripped


It required defer building ILCompiler.csproj until after apphost sfxproj. Also, PackageRID is used to restore the "host" package to build the target, while OutputRID is used to create package for the target, which is what we need here.

Fix #104497

@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Jul 17, 2024
@am11 am11 force-pushed the feature/freebsd-port/outputrid branch 2 times, most recently from 4cd8016 to ff4d675 Compare July 17, 2024 04:10
@am11 am11 force-pushed the feature/freebsd-port/outputrid branch from 8422e48 to 06128bf Compare July 17, 2024 09:18
@am11 am11 marked this pull request as ready for review July 17, 2024 09:18
@am11 am11 force-pushed the feature/freebsd-port/outputrid branch from 06128bf to 9afaa0c Compare July 17, 2024 09:26
@am11 am11 added the os-freebsd FreeBSD OS label Jul 17, 2024
@@ -529,13 +528,17 @@

<!-- Packs sets -->

<ItemGroup>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tricky ordering of the different build steps looks fragile.

I am wondering whether it would be better to use multiple build.sh invocations to bootstrap the FreeBSD cross build. The first invocation would skip building parts that depend on last-known-good packages for target arch, the second invocation would then build the rest and use the packages produced by the first invocation.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, it is fragile as in, when we are not building packs subset, it fails the build on platforms using PublishSingleFile. Pulling Microsoft.NETCore.App.Host.sfxproj in clr subset is also not an option because that depends on libs+host to build first.

There is an existing bug that on the systems where we use PublishAot for ILCompiler, it still looks for apphost (#80154 (comment), it was actually a regression in .net9 p1). That makes the refactoring those parts a degree more delicate.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jkotas, the ILCompiler.csproj build will be now deferred until after packs are built, only for freebsd/illumos/haiku OS set which do not provide Microsoft SDK. This requires single invocation of ./build.sh.

The current state of this PR (351856c) is such that if we still don't like Subsets.props changes, then we can revert Subsets.props (alone) and document the following steps for the devs of those platforms:

# invocation 1 - build everything without ILCompiler support
$ ./build.sh -os freebsd -p:NativeAotSupported=false

# invocation 2 - build clr.tools and packs subsets again without NativeAotSupported
#                condition so ILCompiler.csproj can use live-built apphost.
$ ./build.sh clr.tools+packs -os freebsd

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally, the cross-arch and cross-OS bootstrapping builds would produce a full-featured bits that are equivalent (including how the tools get published) to the natively compiled packages. I think achieving it via multiple invocations of build.sh flow would be easier to maintain than the ordering of the individual build steps.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Thefrank, @sec, basically this is the new workflow for community platforms for cross-OS build (e.g. for freebsd on linux): #105004 (comment) (two steps). I tested it with this workflow https://github.com/am11/CrossRepoCITesting/actions/runs/10845732989/workflow, and artifacts are https://github.com/am11/CrossRepoCITesting/releases/tag/freebsd_10845732989. The ilc executable in runtime.freebsd-x64.Microsoft.DotNet.ILCompiler.10.0.0-ci.nupkg shows:

$ unzip ~/Downloads/runtime.freebsd-x64.Microsoft.DotNet.ILCompiler.10.0.0-ci.nupkg
$ find . -name ilc -exec file {} \; | fold
./tools/ilc: ELF 64-bit LSB pie executable, x86-64, version 1 (FreeBSD), dynamic
ally linked, interpreter /libexec/ld-elf.so.1, for FreeBSD 14.0 (1400097), FreeB
SD-style, BuildID[sha1]=235f8e6220cbf08ec8c1966a415f82f6ed072e4e, with debug_inf
o, not stripped

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$ ./build.sh -os freebsd -p:NativeAotSupported=false
$ ./build.sh clr.tools+packs -os freebsd

I think we need to have a clean separation of responsibilities between the two commands. Otherwise, I expect it will collide with where #107772 is trying to go. A good split may be:

  • The first command should be partial build that produces packages that the full build expects to come from LKG toolsets when there is one for target platform
  • The second command should be a full build that uses the packages produced by the first command

We can define a new subset for the first command to make the command line UX simpler and stable over time. This subset would not be on by default. Building this subset can drop the packages in a new directory. The full build can automatically pick the LKG toolset from there if the directory exists.

Ideally, it would be even possible for the first command to be debug/checked build and the second command to be release build or vice versa, and it should work for non-cross builds too.

@agocke Thoughts?

@agocke
Copy link
Member

agocke commented Sep 11, 2024

FYI, we're having some internal discussions about the live build as a whole. This is a large amount of complexity, vs always using the LKG build, and we're not sure it's worth it. There's a decent chance we back out the live build entirely and just use LKG bits for AOT'ing tools in the product.

@am11
Copy link
Member Author

am11 commented Sep 11, 2024

@agocke thanks. This particular change is for those platforms where:

  • it is using cross-OS build (freebsd on linux)
  • UseNativeAotForComponents is false
  • apphost is needed to publish ILCompiler, and apphost being used is for linux, instead of the just-built freebsd.

I think this is a bit special mode, under which only community-supported platforms are currently falling (freebsd, illumos, haiku).

@@ -529,13 +528,17 @@

<!-- Packs sets -->

<ItemGroup>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally, the cross-arch and cross-OS bootstrapping builds would produce a full-featured bits that are equivalent (including how the tools get published) to the natively compiled packages. I think achieving it via multiple invocations of build.sh flow would be easier to maintain than the ordering of the individual build steps.

@am11 am11 force-pushed the feature/freebsd-port/outputrid branch from ca7c2f6 to 5903321 Compare September 13, 2024 18:38
@am11 am11 requested a review from jkotas September 13, 2024 19:37
@@ -1,18 +1,21 @@
<Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
<OutputPath>$(RuntimeBinDir)ilc/</OutputPath>
<RuntimeIdentifier>$(PackageRID)</RuntimeIdentifier>
<RuntimeIdentifier Condition="'$(_IsPublishing)' != 'true'">$(PackageRID)</RuntimeIdentifier>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where is the _IsPublishing property set?

@agocke's comment #107772 (comment) makes it sound like the _IsPublishing is not set as expected in this project.

Copy link
Member

@agocke agocke Sep 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right now the dotnet CLI sets _IsPublishing when running dotnet publish but it's everyone else's responsibility to set it when running the Publish target (I don't like this behavior, fyi). As far as I can tell, ILCompiler doesn't guarantee this is set, currently.

@@ -529,13 +528,17 @@

<!-- Packs sets -->

<ItemGroup>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$ ./build.sh -os freebsd -p:NativeAotSupported=false
$ ./build.sh clr.tools+packs -os freebsd

I think we need to have a clean separation of responsibilities between the two commands. Otherwise, I expect it will collide with where #107772 is trying to go. A good split may be:

  • The first command should be partial build that produces packages that the full build expects to come from LKG toolsets when there is one for target platform
  • The second command should be a full build that uses the packages produced by the first command

We can define a new subset for the first command to make the command line UX simpler and stable over time. This subset would not be on by default. Building this subset can drop the packages in a new directory. The full build can automatically pick the LKG toolset from there if the directory exists.

Ideally, it would be even possible for the first command to be debug/checked build and the second command to be release build or vice versa, and it should work for non-cross builds too.

@agocke Thoughts?

@agocke
Copy link
Member

agocke commented Sep 13, 2024

I think we need to have a clean separation of responsibilities between the two commands. Otherwise, I expect it will collide with where #107772 is trying to go. A good split may be:

Sorry, I'm not really following the difference between those two commands.

My expectation, looking at it from a naive perspective, is that the difference between the first and second commands is primarily that -p:NativeAotSupported=false is set -- which to me would indicate that everything that might use Native AOT does the same thing, but instead uses trimmed single-file + R2R.

I don't really understand the "second build uses the first build" part.

@jkotas
Copy link
Member

jkotas commented Sep 14, 2024

looking at it from a naive perspective, is that the difference between the first and second commands is primarily that -p:NativeAotSupported=false is set -- which to me would indicate that everything that might use Native AOT does the same thing, but instead uses trimmed single-file + R2R.

NativeAotSupported property controls whether to build native AOT packages. It does not directly control whether to AOT compile tools during the build that is controlled by UseNativeAotForComponents property. UseNativeAotForComponents is false when cross-building from Linux to FreeBSD, so nothing will get AOT compiled when executing these commands. Also, these commands are made to work by strategically placed conditions in the build scripts, like this one.

Let me state the problem that I am trying to solve: We agreed to use LKG runtime and AOT compilers for all components that #107772 is about. Also, we said that it would be nice to have an option to use live runtime and AOT compilers as an option that this PR is about. What should be the UX for that option?

My proposal is that it should be a sequence of two build.cmd/.sh script invocations. The first invocation is a special build flow that builds just the runtime and AOT compiler bits that come from LKG by default. The second invocation is a regular build flow except that it uses runtime and AOT compilers produced by the first invocation instead of the LKG one.

@am11
Copy link
Member Author

am11 commented Sep 14, 2024

@jkotas @agocke, this issue is solving #104497 for freebsd (with no pre-built SDK in Microsoft feed). The issue is about wrong "apphost package" (linux) is being used for freebsd (and not wrong ILCompiler package). First solution was to change the order of subset and use live apphost without breaking invocation, it worked (without breaking existing platforms) but changing the subset order was not clean. Now we have split the two steps; first build the product without publishing ILCompiler. In second round, once the apphost package is built in first invocation, we use that one to publish ILCompiler project.

Please CMIIW, but I think LKG change is not improving #104497 scenario, right?

crossgen2_publish.csproj is a good separation which splits publishing and building concerns so package restore (PackageRID) and package creation (OutputRIID) are kept separate. If we switch ILCompiler to the same scheme, then we won't be needing some of these twists and turns.

@jkotas
Copy link
Member

jkotas commented Sep 14, 2024

I agree with you that live build is better in some situations, like enabling new platforms. Live build is worse for day-to-day development.

crossgen2 publishing added to test build script in #106965 takes a while on debug builds. It made day-to-day development experience worse that triggered a discussion on our Teams channel about why we are using live build for crossgen2. This PR is outcome of that discussion.

We use a mix of LKG builds and live builds for tools today. The goal of this PR is to consistently use LKG build everywhere by default.

We should have an option to use live build. Enabling this option should switch all tools to use live build and it should not change other settings to make things work - for example, it should not turn off PublishAot. My comment at #105004 (comment) is a proposal for how this can work.

HTH

@am11
Copy link
Member Author

am11 commented Sep 14, 2024

@jkotas, thanks for the explanation. The first invocation with NativeAotSupported=<default for platform> will always produce the incorrect binary with --os freebsd --cross (linux ELF instead of freebsd ELF #104497). The second invocation will first need to undo the first invocation's work like evicting the cache (stuff in obj/ and passing /t:Rebuild) so the project doesn't get skipped by msbuild.

Wouldn't it be better to skip publishing the incorrect binary in the first place (the current state of this PR)?

@jkotas
Copy link
Member

jkotas commented Sep 14, 2024

I think that the option to use live build for tools should be an explicit gesture. Here are examples of scenarios that I would like to see it handle (open to suggestion for a better name of -buildtoolsruntime):

  • Bootstrap community supported platform:
./build.sh -os freebsd -c release -buildtoolsruntime
./build.sh -os freebsd -c release
  • Produce release build of the AOT compilers that run on live checked build of the runtime (useful for debugging purposes):
./build.sh -c checked -buildtoolsruntime
./build.sh -c release

The first invocation with NativeAotSupported= will always produce the incorrect binary

-buildtoolsruntime in my example does not actually need to produce the native FreeBSD tools. The native FreeBSD tools are not used by the second step.

One problem is that the ilc and the runtime pack are in one package today. We can solve that by allowing -buildtoolsruntime to produce partial package or by unconditionally using PublishAotUsingRuntimePack for the live tools runtime.

The second invocation will first need to undo the first invocation's work like evicting the cache

This can be handled by -buildtoolsruntime overriding the output directory in places with conflicts.

@am11
Copy link
Member Author

am11 commented Sep 14, 2024

  • Bootstrap community supported platform:
./build.sh -os freebsd -c release -buildtoolsruntime
./build.sh -os freebsd -c release

There are three variants:

  1. dotnet ilc.dll using live aot runtime build + libs
  2. ilc published with PublishSingleFile using live or LKG aot runtime + libs
  3. ilc published with PublishAot using live or LKG aot runtime + libs

(actually there is another; if we incorporate PublishReadyToRun, but it's tied with PublishingSingleFile)

I think for ILCompiler.csproj (NativeAotSupported=true), we should skip variant 2. AFAIK, variant 2 (which this PR is about) brings unnecessary overhead for ILCompiler; because when we are building ILCompiler, we know that target platform has NativeAotSupported=true, which is different than other stuff like crossgen2_publish.csproj src/native/managed etc. with optional PublishAot or PublishSingleFile. IOW, variant-2 is only meaningful for projects other than ILCompiler for platforms which do not support AOT (currently evaluate to NativeAotSupported=false).

Then the first command from your example ./build.sh -os freebsd -c release -buildtoolsruntime will only be dealing with building ilc.dll and second command ./build.sh -os freebsd -c release will be publishing ILCompiler with variant 3 (PublishAot=true); using the cross build mechanism implemented in BuildIntegration.

@jkotas
Copy link
Member

jkotas commented Sep 14, 2024

Yes, if the target platform has NativeAotSupported=true, -buildtoolsruntime needs to build native aot toolset only since that's the only part needed from LKG in that case in a regular build. -buildtoolsruntime should build both ilc and matching native aot runtime pack. Otherwise, it would not be possible to mix and match build flavors (my second example).

If the binaries built by the first and second command overlap exactly (it should be the case in the first example), the regular incremental build mechanism should be able to detect that and skip rebuilding them in the second command.

@am11
Copy link
Member Author

am11 commented Sep 14, 2024

Yes, if the target platform has NativeAotSupported=true, -buildtoolsruntime needs to build native aot toolset only since that's the only part needed from LKG in that case in a regular build.

One problem is that Build target on ILCompiler.csproj is triggering Publish target, and publishing aot toolset requires freebsd apphost which needs to be published first. AFAICT, without decoupling them (build only and publish only), the first step would need to build the aot runtime and libraries for the second step to complete the package.

crossgen2.csproj (for build only), crossgen2_inbuild.csproj (for cross build) and crossgen2_publish.csproj (for publishing) solve this problem by separating these concerns cleanly: #92677 (comment). ILCompiler.csproj is monolithic in nature compared to crossgen2.

@jkotas
Copy link
Member

jkotas commented Sep 15, 2024

the first step would need to build the aot runtime and libraries for the second step to complete the package.

I think it should work like that even if ILCompiler project is decoupled. Otherwise, my second example (use live checked runtime for release build of the compilers) would not work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-NativeAOT-coreclr community-contribution Indicates that the PR has been added by a community member os-freebsd FreeBSD OS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cross built ILCompiler NuGet contains HostOS (Linux) ELFs not TargetOS (FreeBSD) ELFs
4 participants