-
Notifications
You must be signed in to change notification settings - Fork 448
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
[release/6.0.1xx] Arm support #13378
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -138,7 +138,7 @@ | |
</Target> | ||
|
||
<Target Name="GenerateRootFs" Condition="'$(OS)' != 'Windows_NT'"> | ||
<Exec Condition="$(Platform.Contains('arm')) AND '$(Platform)' != 'armel' AND '$(BuildArchitecture)' != 'arm64'" Command="$(ArmEnvironmentVariables) $(ProjectDir)cross/build-rootfs.sh" /> | ||
<Exec Condition="$(Platform.Contains('arm')) AND '$(Platform)' != 'armel' AND '$(BuildArchitecture)' != 'arm64' AND '$(BuildArchitecture)' != 'arm'" Command="$(ArmEnvironmentVariables) $(ProjectDir)cross/build-rootfs.sh" /> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We really ought to have some other way of setting up cross compilation rather than inferring it based on architecture :( There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Indeed! Also, now that Plaform.Contains('arm') excludes There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I tried exploring this suggestion - but as I don't know enough about cross compilation, I'm not comfortable proposing a solution. Would you think it better to open an issue to keep track of this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, it's totally unsuitable for fixing as part of this PR. Filed https://github.com/dotnet/installer/issues/13441 |
||
<Exec Condition="'$(Platform)' == 'armel'" Command="$(ArmEnvironmentVariables) $(ProjectDir)cross/armel/tizen-build-rootfs.sh" /> | ||
</Target> | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(This suggestion is out of scope for this PR)
We use this idiom in a few repos and I think we got it from runtime? IIRC, this was done so we could still be able to use Visual Studio (an x86) application and have it correctly target
x64
. But that little bit of default behaviour means every other architecture needs to be hard-coded here separately. Perhaps we should consider handling Visual Studio and/or Windows here specifically and let everything else default to$(BuildArchitecture)
?Theoretically, through mono, we also support sparc, ppc and mips.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does one detect visual studio use? I'm playing with $([MSBuild]::IsOSPlatform('WINDOWS')) for that specific case, but idk if that covers every case where Visual Studio / Windows is used.