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

Fix winarm64 crossdac build #112594

Merged
merged 5 commits into from
Feb 20, 2025
Merged

Conversation

steveisok
Copy link
Member

#112256 added the ability to cross compile android on windows. This regressed the win-arm64 crossdac build because it stopped passing CMAKE_TARGET_ARCH and CMAKE_TARGET_OS for cross component builds where the host and target os's match.

This change makes sure the cmake variables are passed in all cases except when the hostos isn't passed in or is
windows (host == target).

dotnet#112256 added the ability to cross compile android on windows. This
regressed the win-arm64 crossdac build because it stopped passing CMAKE_TARGET_ARCH and CMAKE_TARGET_OS for
cross component builds where the host and target os's match.

This change makes sure the cmake variables are passed in all cases except when the hostos isn't passed in or is
windows (host == target).
@Copilot Copilot bot review requested due to automatic review settings February 15, 2025 02:46

Choose a reason for hiding this comment

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

Copilot wasn't able to review any files in this pull request.

Files not reviewed (1)
  • src/coreclr/build-runtime.cmd: Language not supported
@steveisok
Copy link
Member Author

@jkotas I think we can take this instead of #112553

@jkotas jkotas requested a review from jkoritzinsky February 15, 2025 03:59
@steveisok steveisok force-pushed the fix-winarm64-crossdac-build branch from 7d20179 to 1db7ce4 Compare February 20, 2025 00:35
@@ -499,7 +499,7 @@ if(NOT CLR_CMAKE_TARGET_BROWSER AND NOT CLR_CMAKE_TARGET_WASI)
set(CMAKE_POSITION_INDEPENDENT_CODE ON)
endif()

if (CLR_CMAKE_TARGET_ANDROID)
if (CLR_CMAKE_TARGET_ANDROID AND NOT CLR_CROSS_COMPONENTS_BUILD)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (CLR_CMAKE_TARGET_ANDROID AND NOT CLR_CROSS_COMPONENTS_BUILD)
if (CLR_CMAKE_HOST_ANDROID)

?


if /i "%__TargetOS%" == "android" (
if not "%__HostOS%" == "" (
set "__TargetOS=!__HostOS!"
Copy link
Member

Choose a reason for hiding this comment

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

If command line says -os android -hostos windows, this line is going to override the requested configuration to -os windows -hostos windows. Why would we want to do that? If the caller of the script asks for android targeting binary, we should build an android targeting binary and not ignore what the command line asked for.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I think that was another part of the workaround. I'll remove it and see.

Copy link
Member Author

Choose a reason for hiding this comment

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

ok, I remember... We need a host built JIT for live builds of crossgen / ILC. There may be other reasons I'm not aware of.

The approach is equivalent of what we do on non-windows platforms:

Executing "/Users/runner/work/1/s/src/coreclr/build-runtime.sh" -cmakeargs "-DCLR_CROSS_COMPONENTS_BUILD=1" -arm64 -release -ci -os ios -hostarch x64 -hostos osx -outputrid ios-arm64 -subdir x64 -cmakeargs "-DCLR_DOTNET_HOST_PATH=/Users/runner/work/1/s/.dotnet/dotnet" -component alljits

And where it gets flipped... It's arranged slightly different, but nonetheless the same outcome.

if [[ -z "$__HostOS" ]]; then
__HostOS=$__TargetOS
fi

Copy link
Member

Choose a reason for hiding this comment

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

if [[ -z "$__HostOS" ]]; then
__HostOS=$__TargetOS
fi

This condition makes sense. It sets HostOS to TargetOS when the HostOS was not specified (ie it sets the default). Can we do it the same way here? Set the default if the value is not specified instead of overriding what was specified on the command line.

Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

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

LGTM, assuming that it works as expected. Thank you!

@jkotas jkotas merged commit fb6b045 into dotnet:main Feb 20, 2025
165 of 170 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants