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

[nativeaot] Add Native AOT cross-build support for iOS-like platforms #88242

Merged
merged 5 commits into from
Jul 11, 2023

Conversation

kotlarmilos
Copy link
Member

@kotlarmilos kotlarmilos commented Jun 30, 2023

This PR adds support for the Native AOT cross-build targeting iOS-like platforms. When building for the target platforms, only the jitinterface component is installed, while clrjit is skipped since it is not required for the target platform. The jitinterface, aotsdk, and native libraries components are built for the target platforms, while jitinterface and clrjit components are built for the host as cross-components.

Additionally, this PR makes the cross build path to be a constant (/host), to allow both cross-arch and cross-os scenarios. Previously, the path was architecture-specific and did not support cross-os scenarios where the target and host architectures are the same.

The updated cross build path caused a lot of regressions and would require updating the tests to match the new path. The idea is to use the architecture-specific path in the cross-os scenarios as well.

It should unblock #87260 and #87773.

@ghost
Copy link

ghost commented Jun 30, 2023

Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas
See info in area-owners.md if you want to be subscribed.

Issue Details

This PR adds support for the Native AOT cross-build targeting iOS-like platforms. When building for the target platforms, only the jitinterface component is installed, while clrjit is skipped since it is not required for the target platform. The jitinterface, aotsdk, and native libraries components are built for the target platforms, while jitinterface and clrjit components are built for the host as cross-components.

Additionally, this PR makes the cross build path to be a constant (/host), to allow both cross-arch and cross-os scenarios. Previously, the path was architecture-specific and did not support cross-os scenarios where the target and host architectures are the same.

It should unblock #87260 and #87773.

Author: kotlarmilos
Assignees: kotlarmilos
Labels:

area-NativeAOT-coreclr

Milestone: 8.0.0

@@ -109,6 +109,9 @@ __ConfigTriplet="$__TargetOS.$__TargetArch.$__BuildType"
if [[ "$__TargetOS" == "linux-bionic" ]]; then
__ConfigTriplet="linux.$__TargetArch.$__BuildType"
fi
if [[ ! -z "$__OutputRIDOS" ]]; then
__ConfigTriplet="$__OutputRIDOS.$__TargetArch.$__BuildType"
Copy link
Member

Choose a reason for hiding this comment

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

Instead of introducing this override for output path, would it be better to keep the compiler in its default path and invoke it from there?

Copy link
Member Author

@kotlarmilos kotlarmilos Jul 10, 2023

Choose a reason for hiding this comment

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

In that case, the default path for the cross-compiler is $(TargetOS).$(TargetArchitecture).$(Configuration)/$(BuildArchitecture), while the jitinterface and clrjit cross-components are generated in $(HostOS).$(TargetArchitecture).$(Configuration)/$(BuildArchitecture), and these components cannot be found.

<CoreCLRArtifactsPath Condition="'$(CoreCLRArtifactsPath)' == ''">$(RuntimeBinDir)$(CrossHostArch)</CoreCLRArtifactsPath>

When building cross-components for cross-arch scenarios, the DCLR_CMAKE_TARGET_ARCH is used for the output path.

if [[ "$__TargetArch" != "$__HostArch" ]]; then
__CMakeArgs="-DCLR_CMAKE_TARGET_ARCH=$__TargetArch $__CMakeArgs"
fi

One approach that I will try is to introduce the -hostos parameter along with -hostarch to indicate the target platform for the cross-components build. Another approach would be to adjust the components path in the _crossarch projects.

Copy link
Member

@MichalStrehovsky MichalStrehovsky left a comment

Choose a reason for hiding this comment

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

Looks good otherwise! Thanks for cleaning it up!

@@ -267,6 +267,22 @@
Category="clr" />
</ItemGroup>

<!-- Build the CoreCLR cross tools when we're doing a cross OS build for apple device targets -->
Copy link
Member

Choose a reason for hiding this comment

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

Could this be folded into the above Build the CoreCLR cross tools when we're doing a cross build block? We currently exclude it with '$(TargetsMobile)' != 'true' but it looks pretty much the same. The additional TargetOS and OutputRIDOS properties will probably not harm anything if we add them.

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 so. Initially, I didn't want to extend the cross-build for unsupported scenarios.

@@ -336,8 +332,8 @@
<ProjectToBuild Include="$(CoreClrProjectRoot)tools\aot\ILCompiler\ILCompiler.csproj" Category="clr" Condition="'$(NativeAotSupported)' == 'true'" />
<ProjectToBuild Include="$(CoreClrProjectRoot)nativeaot\BuildIntegration\BuildIntegration.proj" Category="clr" Condition="'$(NativeAotSupported)' == 'true'" />

<ProjectToBuild Condition="'$(NativeAotSupported)' == 'true' and ('$(CrossBuild)' == 'true' or '$(BuildArchitecture)' != '$(TargetArchitecture)')" Include="$(CoreClrProjectRoot)tools\aot\ILCompiler\ILCompiler_crossarch.csproj" Category="clr" />
<ProjectToBuild Condition="'$(TargetArchitecture)' != '$(BuildArchitecture)'" Include="$(CoreClrProjectRoot)tools\aot\crossgen2\crossgen2_crossarch.csproj" Category="clr" />
<ProjectToBuild Condition="'$(NativeAotSupported)' == 'true' and ('$(CrossBuild)' == 'true' or '$(BuildArchitecture)' != '$(TargetArchitecture)' or ('$(HostOS)' != '$(TargetOS)' and '$(TargetsAppleMobile)' == 'true'))" Include="$(CoreClrProjectRoot)tools\aot\ILCompiler\ILCompiler_crossarch.csproj" Category="clr" />
Copy link
Member

Choose a reason for hiding this comment

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

Do we need the '$(TargetsAppleMobile)' == 'true' condition here and below? It feels like extending it to HostOS != TargetOS should be general goodness.

eng/build.sh Outdated Show resolved Hide resolved
@@ -25,7 +25,7 @@
<CoreCLRSharedFrameworkDir>$([MSBuild]::NormalizeDirectory('$(CoreCLRArtifactsPath)', 'sharedFramework'))</CoreCLRSharedFrameworkDir>
<CoreCLRCrossgen2Dir>$([MSBuild]::NormalizeDirectory('$(CoreCLRArtifactsPath)', 'crossgen2'))</CoreCLRCrossgen2Dir>
<CoreCLRILCompilerDir>$([MSBuild]::NormalizeDirectory('$(CoreCLRArtifactsPath)', 'ilc-published'))</CoreCLRILCompilerDir>
<CoreCLRCrossILCompilerDir Condition="'$(CrossBuild)' == 'true' or '$(BuildArchitecture)' != '$(TargetArchitecture)'">$([MSBuild]::NormalizeDirectory('$(CoreCLRArtifactsPath)', '$(BuildArchitecture)', 'ilc'))</CoreCLRCrossILCompilerDir>
<CoreCLRCrossILCompilerDir Condition="'$(CrossBuild)' == 'true' or '$(BuildArchitecture)' != '$(TargetArchitecture)' or ('$(HostOS)' != '$(TargetOS)' and '$(TargetsAppleMobile)' == 'true')">$([MSBuild]::NormalizeDirectory('$(CoreCLRArtifactsPath)', '$(BuildArchitecture)', 'ilc'))</CoreCLRCrossILCompilerDir>
Copy link
Member

Choose a reason for hiding this comment

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

Same question about the TargetsAppleMobile condition.

@@ -71,7 +71,7 @@
<CoreCLRSharedFrameworkDir>$([MSBuild]::NormalizeDirectory('$(CoreCLRArtifactsPath)','sharedFramework'))</CoreCLRSharedFrameworkDir>
<CoreCLRSharedFrameworkPdbDir>$([MSBuild]::NormalizeDirectory('$(CoreCLRSharedFrameworkDir)','PDB'))</CoreCLRSharedFrameworkPdbDir>
<CoreCLRCrossTargetComponentDir
Condition="'$(CoreCLRCrossTargetComponentDirName)' != ''">$([MSBuild]::NormalizeDirectory('$(CoreCLRArtifactsPath)','$(CoreCLRCrossTargetComponentDirName)','sharedFramework'))</CoreCLRCrossTargetComponentDir>
Condition="'$(CoreCLRCrossTargetComponentDirName)' != ''">$([MSBuild]::NormalizeDirectory('$(CoreCLRArtifactsPath)','$(BuildArchitecture)','sharedFramework'))</CoreCLRCrossTargetComponentDir>
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need this change (I was not able to find a use of this property that looked relevant)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not needed, it was a temporary change.

@@ -42,7 +42,7 @@
</PropertyGroup>

<ItemGroup>
<LinkerArg Include="--target=$(TargetTriple)" Condition="'$(TargetOS)' != 'osx' and '$(TargetTriple)' != ''" />
<LinkerArg Include="--target=$(TargetTriple)" Condition="'$(TargetOS)' != 'osx' and '$(TargetsAppleMobile)' != 'true' and '$(TargetTriple)' != ''" />
Copy link
Member

Choose a reason for hiding this comment

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

Would this work?

Suggested change
<LinkerArg Include="--target=$(TargetTriple)" Condition="'$(TargetOS)' != 'osx' and '$(TargetsAppleMobile)' != 'true' and '$(TargetTriple)' != ''" />
<LinkerArg Include="--target=$(TargetTriple)" Condition="'$(_IsApplePlatform)' != 'true' and '$(TargetTriple)' != ''" />

kotlarmilos and others added 2 commits July 10, 2023 16:15
Co-authored-by: Michal Strehovský <MichalStrehovsky@users.noreply.github.com>
Copy link
Member

@MichalStrehovsky MichalStrehovsky left a comment

Choose a reason for hiding this comment

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

Looks good, thank you! Cc @jkoritzinsky

@kotlarmilos
Copy link
Member Author

@@ -104,7 +104,7 @@ build_native()
echo "Error: Unknown Android architecture $hostArch."
exit 1
fi
elif [[ "$__TargetOS" == iossimulator ]]; then
Copy link
Member Author

Choose a reason for hiding this comment

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

@akoeplinger it looks like the $__TargetOS was used by mistake instead of $targetOS, please check.

Copy link
Member

@steveisok steveisok left a comment

Choose a reason for hiding this comment

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

LGTM!

@kotlarmilos kotlarmilos merged commit 14e8b82 into dotnet:main Jul 11, 2023
165 of 170 checks passed
@ghost ghost locked as resolved and limited conversation to collaborators Aug 13, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants