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

Prevent Arm64 CrossDac builds running on Arm64. #88467

Merged
merged 3 commits into from
Jul 17, 2023

Conversation

c272
Copy link
Contributor

@c272 c272 commented Jul 6, 2023

Currently, when building the CLR on win-arm64, CrossDac builds for Linux with an (incorrect) host arch of x64 are attempted, failing the build. This patch prevents those builds running on systems with an identical host architecture to the cross target.

Currently, when building the CLR on win-arm64, CrossDac builds for
Linux with an (incorrect) host arch of x64 are attempted, failing
the build. This patch prevents those builds running on systems with
an identical host architecture to the cross target.

TEST_LABEL: aarch64
TEST_IMG: ubuntu/dotnet-build
TEST_CMD: safe ./projects/dotnet/build-runtime.sh
Jira: ENTLLT-6600

Change-Id: I289e23c85376d238408799df0a573fb659e1522f
@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Jul 6, 2023
@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Jul 6, 2023
@a74nh
Copy link
Contributor

a74nh commented Jul 6, 2023

@kunalspathak and looks like @jkoritzinsky did a bunch of cross build changes in this area

@am11 am11 added area-Infrastructure-coreclr and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Jul 6, 2023
@ghost
Copy link

ghost commented Jul 6, 2023

Tagging subscribers to this area: @hoyosjs
See info in area-owners.md if you want to be subscribed.

Issue Details

Currently, when building the CLR on win-arm64, CrossDac builds for Linux with an (incorrect) host arch of x64 are attempted, failing the build. This patch prevents those builds running on systems with an identical host architecture to the cross target.

Author: c272
Assignees: -
Labels:

area-Infrastructure-coreclr, community-contribution

Milestone: -

The previous patch was insufficient at preventing crossbuild of
x64 onto x86, which also fails. This additional patch addresses
that issue by explicitly checking for that scenario.
@@ -276,7 +276,7 @@
<CrossDacHostArch Condition="'$(TargetArchitecture)' == 'arm'">x86</CrossDacHostArch>
</PropertyGroup>

<ItemGroup Condition="$(_subset.Contains('+linuxdac+')) and $([MSBuild]::IsOsPlatform(Windows)) and '$(TargetArchitecture)' != 'x86'">
<ItemGroup Condition="$(_subset.Contains('+linuxdac+')) and $([MSBuild]::IsOsPlatform(Windows)) and '$(BuildArchitecture)' != '$(TargetArchitecture)' and ('$(BuildArchitecture)' != 'x64' or '$(TargetArchitecture)' != 'x86')">
Copy link
Member

Choose a reason for hiding this comment

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

This isn't correct - x64 Windows will build the x64 linux crossdac.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Have had another go at this -- this time kept the check identical but excluded architectures other than x64 and x86. Since L274-277 seem to assume this is the case anyway, I've made the assumption this will be sufficient, but I'm unsure in exactly which scenarios this should be running.

Let me know if this isn't correct, and I'll try and correct again. Apologies.

Copy link
Member

Choose a reason for hiding this comment

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

No need to apologize. I should have also been more descriptive. This won't break the official build. That being said - it's something that we should fix at some point - so that it's possible to use arm64 windows debuggers to analyze linux dumps.

@ghost ghost added the needs-author-action An issue or pull request that requires more info or actions from the author. label Jul 6, 2023
@ghost ghost removed the needs-author-action An issue or pull request that requires more info or actions from the author. label Jul 7, 2023
@c272 c272 force-pushed the dotnet/arm-crossdac branch from 803def5 to 1083f5d Compare July 7, 2023 10:32
@c272 c272 requested a review from hoyosjs July 7, 2023 12:25
@a74nh
Copy link
Contributor

a74nh commented Jul 14, 2023

Quick ping on this. Is this patch correct now?

Little confused by the 7d running test, but I don't think it's this patch.

It would be nice to have this patch as it would enable us to build on Windows on Arm in our local setup.

@hoyosjs hoyosjs merged commit 722344e into dotnet:main Jul 17, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Aug 17, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Infrastructure-coreclr community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants