-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Add default toolchains for MacOS. #20310
Conversation
8babc97
to
346f58f
Compare
@big-guy possibly of interest to you as you recently fixed the arm-v8/aarch64 confusion in native builds? |
Thanks @prbprbprb We have an experimental M1-based pipeline I can run this through |
Thanks! I kinda forgot about the docs changes, I'll get on that. And do you have any pointer to where I should be adding tests? |
@prbprbprb take a look at Line 312 in 89ea0aa
I think making that also check for aarch64 would do it. I think these are the only docs that need to be updated right now: I think there are more things we can do after this, but this is enough to get this merged. Thanks! |
Thanks, I'll work through that! One corner case I forgot - if there are versions of Xcode in the wild old enough not to support the Happy to take follow-on work too as we also want to support ARM Linux better in Conscrypt. |
@prbprbprb we'd like to merge this PR in the next week or so; do you think you'll have time to contribute any tests or look at the corner-case for older Xcode versions? |
I do apologise, kind of swamped with the day job recently. I will have time to look at this PR over the coming weekend but if it's easier for you take over the work in the meantime then you have my blessing. |
Working on it but note that both the |
Unit tests and docs updated. Do we need an integration test? |
Looks like the |
@@ -94,6 +94,8 @@ public AbstractGccCompatibleToolChain(String name, BuildOperationExecutor buildO | |||
|
|||
target(new Intel32Architecture()); | |||
target(new Intel64Architecture()); | |||
target(new OsxIntelArchitecture()); |
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.
💄 It would be better if we could avoid duplicated architecture. It would be possible to choose different flags by using getPlatform() on the toolchain. The downside of merging Intel64Architecture
and OsxIntelArchitecture
is some tests may need to be adjusted which can be troublesome.
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.
I think you're suggesting three architectures, correct? And the probing for OS-dependent qualities will happen in their implementations?
- Intel32Architecture
- Intel64Architecture
- Arm64Architecture
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.
We could use Arm64Architecture
, but I think for this PR using OsxArm64Architecture
would be enough. Adding arm64
support for all OS is a big can of worm to open.
While looking through #20310 , the author mentioned that some of the existing tests weren't working. Indeed, the implicit assertions buried within a closure were returning false but not failing the tests. I checked with @leonard84 who shared the [modern way](https://spockframework.org/spock/docs/2.1/release_notes.html#_code_argument_constraints_are_treated_as_implicit_assertions) to check these conditions. I've updated the test to use this method and confirm it now passes on CI. Note that I have changed the test conditions to make it pass, but I'm not sure whether this behavior is truly expected or not. As such, I'm open to suggestions/fixes to the production code as well. Note I've seen some flakiness from `o.g.a.i.c.LibrariesSourceGeneratorTest.generated sources can be compiled` on java 8, but I don't see how it's related. Co-authored-by: Kyle Moore <[email protected]>
87a0fca
to
b22ee22
Compare
1d8c1db
to
3230377
Compare
@lacasseio can you have another look? |
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.
LGTM
Fixes gradle#1096 Adds two new default toolschain targets for MacOS x86_64 and arm64 which use Clang's `-arch` argument to target the correct architecture. On MacOS, this is used in preference to the existing `Intel64Architecture` target which passes `-m64` to the toolchain. This is sufficient for `NativeExecutableSpec` to target `osx_x86-64` and `osx_aarch64` on both Intel and M1 Macs. I haven't (yet!) located any tests for this functionality, and so this PR doesn't include any test changes. Happy to add them if you can point me in the right direction. Signed-off-by: Pete Bentley <[email protected]>
Notes: Multi-arch support breaks some previous assumptions, especially as dummyOS is an instance of the current OS and the `TargetPlatformConfiguration`s (existing and added) also depend on the current OS. I worked around it with `@IgnoreIf`s but there ought to be a way to test all configs on all OSes. Cherry-pick and merge changes from PR gradle#20882 Use @requires rather than inverted @IgnoreIf. Also drop spurious blank line that crept in. Signed-off-by: Pete Bentley <[email protected]>
Fixes gradle#1096 Adds two new default toolschain targets for MacOS x86_64 and arm64 which use Clang's `-arch` argument to target the correct architecture. On MacOS, this is used in preference to the existing `Intel64Architecture` target which passes `-m64` to the toolchain. This is sufficient for `NativeExecutableSpec` to target `osx_x86-64` and `osx_aarch64` on both Intel and M1 Macs. I haven't (yet!) located any tests for this functionality, and so this PR doesn't include any test changes. Happy to add them if you can point me in the right direction. Signed-off-by: Pete Bentley <[email protected]> Signed-off-by: Kyle Moore <[email protected]>
3230377
to
60d2688
Compare
@bot-gradle test this |
OK, I've already triggered the following builds for you: |
@bot-gradle test and merge |
Your PR is queued. See the queue page for details. |
OK, I've already triggered a build for you. |
Extension of #20310 , but enables native integration tests on M1. The production code changes are mercifully small: - `MachineArchitecture.java` - defines a new AMD64 attribute - `GenerateXcodeProjectFileTask.java` - samples MachineArchitecture value to generate M1-compatible Xcode project - `default.xcsettings` - modify default Xcode project template to support "new build system" for future compatibility - `AbstractGccCompatibleToolChain.java` - Passes correct compiler flag when building on M1; from contributor in #20310 The remaining changes to tests and fixtures pass on my local environment, nevertheless I had to disable several tests due to the old Xcode versions present on our CI infrastructure. Co-authored-by: Kyle Moore <[email protected]>
Co-authored-by: Kyle Moore <[email protected]>
Fixes gradle-native #1096
Adds two new default toolchain targets for MacOS x86_64 and
arm64 which use Clang's
-arch
argument to target the correctarchitecture. On MacOS, this is used in preference to the
existing
Intel64Architecture
target which passes-m64
to thetoolchain.
This is sufficient for
NativeExecutableSpec
to targetosx_x86-64
andosx_aarch64
on both Intel and M1 Macs.I haven't (yet!) located any tests for this functionality, and
so this PR doesn't include any test changes. Happy to add them
if you can point me in the right direction.
Signed-off-by: Pete Bentley [email protected]
Fixes gradle-native #1096
Context
Ease of use on both Intel and ARM based Macs.
Contributor Checklist
<subproject>/src/integTest
) to verify changes from a user perspective<subproject>/src/test
) to verify logic./gradlew sanityCheck
./gradlew <changed-subproject>:quickTest
Gradle Core Team Checklist