Add default toolchains for MacOS. #20310

prbprbprb · 2022-03-29T13:48:20Z

Adds two new default toolchain 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]

Fixes gradle-native #1096

Context

Ease of use on both Intel and ARM based Macs.

Contributor Checklist

Review Contribution Guidelines
Make sure that all commits are signed off to indicate that you agree to the terms of Developer Certificate of Origin.
Make sure all contributed code can be distributed under the terms of the Apache License 2.0, e.g. the code was written by yourself or the original code is licensed under a license compatible to Apache License 2.0.
Check "Allow edit from maintainers" option in pull request so that additional changes can be pushed by Gradle team
Provide integration tests (under <subproject>/src/integTest) to verify changes from a user perspective
Provide unit tests (under <subproject>/src/test) to verify logic
Update User Guide, DSL Reference, and Javadoc for public-facing changes
Ensure that tests pass sanity check: ./gradlew sanityCheck
Ensure that tests pass locally: ./gradlew <changed-subproject>:quickTest

Gradle Core Team Checklist

Verify design and implementation
Verify test coverage and CI build status
Verify documentation
Recognize contributor in release notes

prbprbprb · 2022-04-01T12:20:52Z

@big-guy possibly of interest to you as you recently fixed the arm-v8/aarch64 confusion in native builds?

big-guy · 2022-04-20T14:59:49Z

Thanks @prbprbprb

We have an experimental M1-based pipeline I can run this through

prbprbprb · 2022-04-20T16:15:39Z

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?

big-guy · 2022-04-20T22:32:11Z

@prbprbprb take a look at

gradle/subprojects/platform-native/src/test/groovy/org/gradle/nativeplatform/toolchain/internal/gcc/AbstractGccCompatibleToolChainTest.groovy

Line 312 in 89ea0aa

def "supplies args for supported architecture for macOS platforms"() {

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:
https://github.com/gradle/gradle/blob/34f6ad81c5f95c3da29eb7a6e273b00344d75b17/subprojects/docs/src/docs/userguide/native/native_software.adoc#using-tool-chains

I think there are more things we can do after this, but this is enough to get this merged.

Thanks!

prbprbprb · 2022-04-21T08:38:11Z

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 -arch probe args we might need to fall back to -m64. I'll talk to some MacOS folk and see :)

Happy to take follow-on work too as we also want to support ARM Linux better in Conscrypt.

DPUkyle · 2022-05-09T16:06:40Z

@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?

prbprbprb · 2022-05-09T16:16:51Z

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.

prbprbprb · 2022-05-15T14:18:35Z

Working on it but note that both the supplies args tests in AbstractGccCompatibleToolChainTest.groovy appear to be currently broken... I can randomly change the expectations in the where: clause and they still pass :)

prbprbprb · 2022-05-15T19:02:52Z

Unit tests and docs updated. Do we need an integration test?

prbprbprb · 2022-05-16T13:54:42Z

do you think you'll have time to [...] look at the corner-case for older Xcode versions?

Looks like the -arch flag has been around on MacOS since at least their PPC days, so I don't think we need to worry about it.

lacasseio · 2022-06-01T14:34:26Z

...in/java/org/gradle/nativeplatform/toolchain/internal/gcc/AbstractGccCompatibleToolChain.java

@@ -94,6 +94,8 @@ public AbstractGccCompatibleToolChain(String name, BuildOperationExecutor buildO

 target(new Intel32Architecture());
 target(new Intel64Architecture());
+ target(new OsxIntelArchitecture());


💄 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.

I think you're suggesting three architectures, correct? And the probing for OS-dependent qualities will happen in their implementations?

Intel32Architecture

Intel64Architecture

Arm64Architecture

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.

@leonard84

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]>

DPUkyle · 2022-07-14T01:05:27Z

@lacasseio can you have another look?

nathan-contino

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]>

…chitecture

@requires

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]>

big-guy · 2022-09-09T12:53:35Z

@bot-gradle test this

bot-gradle · 2022-09-09T12:53:44Z

OK, I've already triggered the following builds for you:

PullRequestFeedback build

DPUkyle · 2022-09-09T17:17:19Z

@bot-gradle test and merge

bot-gradle · 2022-09-09T17:17:29Z

Your PR is queued. See the queue page for details.

bot-gradle · 2022-09-09T22:29:07Z

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]>

prbprbprb mentioned this pull request Mar 30, 2022

Add support to osx-aarch_64 google/conscrypt#1034

Open

prbprbprb force-pushed the osx_toolchain branch 2 times, most recently from 8babc97 to 346f58f Compare April 1, 2022 12:19

big-guy added the from:contributor PR by an external contributor label Apr 1, 2022

ljacomet added the in:native-platform c, cpp, swift and other native languages support, etc label Apr 8, 2022

big-guy added this to the 7.6 RC1 milestone Apr 22, 2022

big-guy self-assigned this Apr 22, 2022

big-guy assigned DPUkyle May 9, 2022

prbprbprb force-pushed the osx_toolchain branch from 346f58f to 73d0af0 Compare May 15, 2022 13:10

prbprbprb force-pushed the osx_toolchain branch from 0427a92 to 1841bd0 Compare May 15, 2022 18:59

gradle deleted a comment from big-guy May 16, 2022

prbprbprb force-pushed the osx_toolchain branch from dcf412b to 9e1de5c Compare May 17, 2022 08:23

gradle deleted a comment from DPUkyle May 31, 2022

DPUkyle mentioned this pull request May 31, 2022

Fix broken assertions for gcc toolchain tests. #20882

Merged

lacasseio reviewed Jun 1, 2022

View reviewed changes

big-guy removed their assignment Jul 1, 2022

gradle deleted a comment from DPUkyle Jul 13, 2022

DPUkyle force-pushed the osx_toolchain branch 2 times, most recently from 87a0fca to b22ee22 Compare July 13, 2022 23:44

DPUkyle force-pushed the osx_toolchain branch 2 times, most recently from 1d8c1db to 3230377 Compare July 14, 2022 00:01

gradle deleted a comment from DPUkyle Jul 14, 2022

DPUkyle requested a review from big-guy July 14, 2022 01:04

gradle deleted a comment from DPUkyle Jul 22, 2022

DPUkyle mentioned this pull request Jul 29, 2022

macOS M1 toolchain integration for aarch64 #21405

Merged

big-guy changed the base branch from master to release August 9, 2022 21:43

big-guy requested a review from nathan-contino as a code owner August 9, 2022 21:43

nathan-contino approved these changes Aug 10, 2022

View reviewed changes

big-guy added the @core Issue owned by GBT Core label Sep 7, 2022

prbprbprb and others added 4 commits September 9, 2022 08:53

Support MacOS-specific flags for Intel64; use MacOS-specific Arm64 ar…

35455d9

…chitecture

big-guy force-pushed the osx_toolchain branch from 3230377 to 60d2688 Compare September 9, 2022 12:53

gradle deleted a comment from DPUkyle Sep 9, 2022

gradle deleted a comment from big-guy Sep 9, 2022

bot-gradle merged commit 23f07b6 into gradle:release Sep 9, 2022

DPUkyle added a commit that referenced this pull request Sep 10, 2022

Recognize contributor of #20310

5b1566d

bot-gradle added a commit that referenced this pull request Sep 12, 2022

Merge pull request #21955 Recognize contributor of #20310

4298bfe

Co-authored-by: Kyle Moore <[email protected]>

DPUkyle mentioned this pull request Sep 29, 2022

C++ on M1 adammurdoch/gradle-launcher-mocks#2

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add default toolchains for MacOS. #20310

Add default toolchains for MacOS. #20310

prbprbprb commented Mar 29, 2022 •

edited

prbprbprb commented Apr 1, 2022

big-guy commented Apr 20, 2022

prbprbprb commented Apr 20, 2022

big-guy commented Apr 20, 2022

prbprbprb commented Apr 21, 2022

DPUkyle commented May 9, 2022

prbprbprb commented May 9, 2022

prbprbprb commented May 15, 2022

prbprbprb commented May 15, 2022

prbprbprb commented May 16, 2022

lacasseio Jun 1, 2022

DPUkyle Jun 3, 2022

lacasseio Jun 14, 2022

DPUkyle commented Jul 14, 2022

nathan-contino left a comment

big-guy commented Sep 9, 2022

bot-gradle commented Sep 9, 2022

DPUkyle commented Sep 9, 2022

bot-gradle commented Sep 9, 2022

bot-gradle commented Sep 9, 2022

Add default toolchains for MacOS. #20310

Add default toolchains for MacOS. #20310

Conversation

prbprbprb commented Mar 29, 2022 • edited

Context

Contributor Checklist

Gradle Core Team Checklist

prbprbprb commented Apr 1, 2022

big-guy commented Apr 20, 2022

prbprbprb commented Apr 20, 2022

big-guy commented Apr 20, 2022

prbprbprb commented Apr 21, 2022

DPUkyle commented May 9, 2022

prbprbprb commented May 9, 2022

prbprbprb commented May 15, 2022

prbprbprb commented May 15, 2022

prbprbprb commented May 16, 2022

lacasseio Jun 1, 2022

Choose a reason for hiding this comment

DPUkyle Jun 3, 2022

Choose a reason for hiding this comment

lacasseio Jun 14, 2022

Choose a reason for hiding this comment

DPUkyle commented Jul 14, 2022

nathan-contino left a comment

Choose a reason for hiding this comment

big-guy commented Sep 9, 2022

bot-gradle commented Sep 9, 2022

DPUkyle commented Sep 9, 2022

bot-gradle commented Sep 9, 2022

bot-gradle commented Sep 9, 2022

prbprbprb commented Mar 29, 2022 •

edited