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 28 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?

@am11 am11 force-pushed the feature/freebsd-port/outputrid branch 6 times, most recently from b5e0e5c to dd30dd1 Compare January 9, 2025 00:46
@am11 am11 force-pushed the feature/freebsd-port/outputrid branch from dd30dd1 to fc810a3 Compare January 9, 2025 01:27
@am11
Copy link
Member Author

am11 commented Jan 9, 2025

@jkotas, @jkoritzinsky, community platform builds are green. Note that freebsd-x64 and linux-loongarch64 set NativeAotSupported as before. If #110688 is merged before this PR, then we can add targetArchitecture==riscv64 condition in Subsets.props and enable it. Otherwise, I will add it in that PR or as a separate one once both are merged.

If we want to enable UseNativeAotForComponents in the CI, then we will need to add a third stage; first stage prepares apphost, second stage uses PublishSingleFile for tools build and the third stage will use tools from second stage to use PublishAot. Please let me know if we want the third stage.

@@ -12,9 +12,11 @@
<GenerateRuntimeConfigurationFiles>true</GenerateRuntimeConfigurationFiles>
<EnableDefaultEmbeddedResourceItems>false</EnableDefaultEmbeddedResourceItems>
<Configurations>Debug;Release;Checked</Configurations>
<UseLocalTargetingRuntimePack Condition="'$(StageTwoBuild)' == 'true'">true</UseLocalTargetingRuntimePack>
Copy link
Member

Choose a reason for hiding this comment

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

Is this a problem for incremental builds? Should the build with LKG target and the build local targeting pack go into separate directories?

Copy link
Member Author

Choose a reason for hiding this comment

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

This project is not built in stage 1, so incremental build does not come into play.

@jkotas
Copy link
Member

jkotas commented Jan 9, 2025

I am not following why we would need 3 stages to enable UseNativeAotForComponents. The tools built by the first stage need to target the build machine architecture only to unblock second stage. We should have everything available to build the tools that target the build machine architecture during the first stage.

am11 added 3 commits January 9, 2025 14:25
This was only needed for Android target on Windows build machine, which is guarded by TargetsLinuxBionic condition.
@am11
Copy link
Member Author

am11 commented Jan 9, 2025

I am not following why we would need 3 stages to enable UseNativeAotForComponents. The tools built by the first stage need to target the build machine architecture only to unblock second stage. We should have everything available to build the tools that target the build machine architecture during the first stage.

@jkotas, I have tested it. Setting UseNativeAotForComponents alone did not use live AOT build as expected, it just published crossgen2/ilc with corehost. Seems like it requires more configurations like we have in tests.singlefile.targets etc. so AOT toolchain can find PrivateSdkAssemblies and FrameworkAssemblies.

@@ -360,9 +360,9 @@
$(CoreClrProjectRoot)tools\PdbChecker\PdbChecker.csproj;
$(CoreClrProjectRoot)tools\AssemblyChecker\AssemblyChecker.csproj" Category="clr" Condition="'$(DotNetBuildSourceOnly)' != 'true'"/>
<!-- skip the architectures that don't have LKG runtime packs -->
<ProjectToBuild Include="$(CoreClrProjectRoot)tools\aot\crossgen2\crossgen2_publish.csproj" Condition="'$(NativeAotSupported)' == 'true'" Category="clr" />
<ProjectToBuild Include="$(CoreClrProjectRoot)tools\aot\crossgen2\crossgen2_publish.csproj" Condition="'$(NativeAotSupported)' == 'true' and '$(StageOneBuild)' != 'true'" Category="clr" />
Copy link
Member Author

Choose a reason for hiding this comment

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

@jkoritzinsky ILCompiler_publish.csproj is not listed in this file (or anywhere else). Was it an oversight or something for the future?

Copy link
Member

Choose a reason for hiding this comment

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

I think there was an intent to hook it up for the Microsoft.DotNet.ILCompiler package, but I think it didn't quite happen. Looks like we still publish from the ILCompiler.csproj project and ship that. I think the build needs to be adjusted to hook this up correctly (use ILCompiler_publish.csproj to publish ilc and remove the publishing logic from ILCompiler.csproj)

@jkotas
Copy link
Member

jkotas commented Jan 9, 2025

Seems like it requires more configurations like we have in tests.singlefile.targets etc. so AOT toolchain can find PrivateSdkAssemblies and FrameworkAssemblies.

This should work by building the NAOT compiler and runtime packs. We may want to force PublishAotUsingRuntimePack to true when these packages are used in the second stage to side-step the problem of building the compiler for the target arch in the first stage.

Replacing PrivateSdkAssemblies and FrameworkAssemblies requires hooking up the rest of the build integration too that often has changes that has to be in sync with the runtime. It gets complicated and fragile quickly. We used to do that, but got rid of it.

@am11
Copy link
Member Author

am11 commented Jan 9, 2025

PublishAotUsingRuntimePack

It gives

/runtime/.dotnet/sdk/10.0.100-alpha.1.24610.7/Sdks/Microsoft.NET.Sdk/targets/Microsoft.NET.Sdk.FrameworkReferenceResolution.targets(497,5):
error NETSDK1082: There was no runtime pack for Microsoft.NETCore.App available for the specified RuntimeIdentifier 'linux-loongarch64'.
[/runtime/src/coreclr/tools/aot/crossgen2/crossgen2_publish.csproj]

We can look into UseNativeAotForComponents separately. Original issue #104497 was about corehost published ilc binary is using linux apphost on freebsd cross build, which is fixed.

<_NativeAotSupportedArch Condition="'$(TargetArchitecture)' == 'x64' or '$(TargetArchitecture)' == 'arm64' or '$(TargetArchitecture)' == 'arm' or '$(TargetArchitecture)' == 'loongarch64' or ('$(TargetOS)' == 'windows' and '$(TargetArchitecture)' == 'x86')">true</_NativeAotSupportedArch>
<NativeAotSupported Condition="'$(_NativeAotSupportedOS)' == 'true' and '$(_NativeAotSupportedArch)' == 'true' and '$(_IsCommunityCrossArchitecture)' != 'true'">true</NativeAotSupported>
<UseNativeAotForComponents Condition="'$(NativeAotSupported)' == 'true' and '$(TargetOS)' == '$(HostOS)' and '$(TargetsLinuxBionic)' != 'true'">true</UseNativeAotForComponents>
<_NativeAotSupportedArch Condition="'$(TargetArchitecture)' == 'x64' or '$(TargetArchitecture)' == 'arm64' or '$(TargetArchitecture)' == 'arm' or '$(TargetArchitecture)' == 'loongarch64' or '$(TargetArchitecture)' == 'riscv64' or ('$(TargetOS)' == 'windows' and '$(TargetArchitecture)' == 'x86')">true</_NativeAotSupportedArch>
Copy link
Member Author

Choose a reason for hiding this comment

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

I think we should invert this condition; blacklist instead of whitelist the unsupported platforms.

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
6 participants