Change default apple_platform_type to macOS #6366

keith · 2018-10-11T21:43:39Z

When using rules for Apple platforms that aren't platform specific, such
as swift_binary, the default of iOS doesn't make sense, instead we
should default this to the current platform, which currently can only be
macOS.

More details about this issue can be found here bazelbuild/rules_swift#51

allevato · 2018-10-11T22:01:05Z

This change would have very broad ranging implications within Google. I believe there is a significant amount of cleanup that would need to be done before we could flip this switch, which is why we haven't done it yet.

keith · 2018-10-11T22:02:30Z

What other rules are affected by this?

allevato · 2018-10-11T22:07:13Z

All the objc_* rules would be affected by this.

The reason for the iOS default is entirely historical (the first users of Apple support in Bazel were building iOS apps, and today it's still the case that that's the majority of our users), so it "made sense" at the time (in fact, iOS was the only supported target for a while).

It's not so much the rules themselves that matter here, but all the implicit assumptions elsewhere that assume that a flagless build will target iOS that would break.

lberki · 2018-10-12T12:49:46Z

I only touched AppleCommandLineOptions only due to my interest in C++ rules and because I did a cleanup out of general desire for cleanliness.

I think it would be worthwhile to eventually unify the configuration distinguisher and the platform type, but that would require adding the "unspecified Apple" platform type. But since I don't actually know much about Apple rules, much less about how this change would affect Google-internal use cases, I'll defer to Sergio.

keith · 2018-10-12T14:51:20Z

Does this mean google internally is using this undocumented flag as well? It seems like we should either add a way for rules to specify this, or publicize this flag

ulfjack · 2018-10-15T13:07:21Z

Yes, the flag is definitely used. It seems to be set on an ios config (in a bazelrc); if everyone is using that, changing the default would be safe.

sergiocampama · 2018-10-15T14:26:09Z

While it is set in the bazelrc for some ios and macos configs, most teams do not use flags when building objc_library targets, which the majority of them are for iOS internally. We would need to set it by default in the bazelrc, without any config setting.

keith · 2018-10-15T15:46:08Z

I assume there would be a problem there too where if you had a target that you built for macOS for tools, it would need to have that overridden (but maybe that's already happening so it would be ok)

keith · 2018-11-20T17:19:10Z

Any thoughts on how we can get to the point where you can build some targets like swift_binary without having to specify this flag? Maybe an alternative is to allow setting the platform type (or just defaulting it) on those rules?

sergiocampama · 2019-01-04T15:52:07Z

One thing that the bazel team could do is to copybara this change so that bazel by default is MACOS, while internally it's still IOS. Another thing would be to make it MACOS by default and add IOS as the default in bazelrc files.

keith · 2019-01-04T17:12:20Z

@aragos do you have any thoughts on the possible solutions here?

keith · 2019-01-04T19:39:36Z

I've updated the tests that were relying on the iOS default to pass --apple_platform_type explicitly.

keith · 2019-01-04T19:40:07Z

src/test/java/com/google/devtools/build/lib/rules/objc/AppleToolchainSelectionTest.java

@@ -41,12 +41,12 @@ public void testToolchainSelectionDefault() throws Exception {

    assertThat(cppConfig.getRuleProvidingCcToolchainProvider().toString())
        .isEqualTo("//tools/osx/crosstool:crosstool");
-    assertThat(cppConfig.getTransformedCpuFromOptions()).isEqualTo("ios_x86_64");
+    assertThat(cppConfig.getTransformedCpuFromOptions()).isEqualTo("darwin_x86_64");


This is the only test I intentionally changed the behavior of, I believe that we want to now test the default is macOS (and therefore is this default arch)

aragos · 2019-01-07T21:01:03Z

Looks good to me in principle, I can take care of explicitly setting the Google-internal default so that this change doesn't cause any behavioral differences there.

We should also test (and fix) any other downstream projects that may be affected, this can be done following this guide: https://github.com/bazelbuild/continuous-integration/blob/master/docs/downstream-testing.md. EDIT: I think this can only be done with owner privileges so I'll take care of this step as well once the tests (see below) are fixed.

Finally there still appear to be some failing tests in Bazel's buildkite, please fix those as well.

Once the two buildkite verifications are done I'll import the PR.

keith · 2019-01-07T21:18:57Z

Sounds good. Quick question @aragos should I follow those word for word and create a new branch with this same change, or can we trigger that build on this branch when we're ready?

aragos · 2019-01-07T21:21:01Z

@keith I believe we'll need a fresh branch with this same change. Feel free to create it and run the downstream projects if you have the necessary permissions, otherwise let me know and I can help.

keith · 2019-01-07T21:31:11Z

Sounds good. I've emailed bazel_dev asking for the right permissions. If I don't get them by the time I'm ready I'll ask, thanks!

When using rules for Apple platforms that aren't platform specific, such as `swift_binary`, the default of iOS doesn't make sense, instead we should default this to the current platform, which currently can only be macOS.

keith · 2019-01-07T21:48:47Z

Oh actually, the recommendation is to create that branch on the bazel repo itself, not my fork, which isn't something I can do without having push access to this repo. I've pushed a branch with the naming suggested in that doc to my fork master...keith:keith-test-feature-platform-default @aragos is this something you can mirror to the bazel repo so we can run the tests (we might want to wait until all other tests are green here first though)

keith · 2019-01-08T00:10:23Z

Ok, it passed, so @aragos I guess we're good to start the others

aragos · 2019-01-08T20:18:07Z

Great! I imported and pushed your changes to https://github.com/bazelbuild/bazel/tree/aragos-test-macos-default-platform and started a buildkite run with that at https://buildkite.com/bazel/bazel-at-head-plus-downstream/builds/735. This may take a while, let's see how it goes.

keith · 2019-01-08T21:04:54Z

Thanks! I don't have access yet but hopefully I'll get it soon.

…

-- Keith Smiley

On Jan 8, 2019, at 12:18, aragos ***@***.***> wrote: Great! I imported and pushed your changes to https://github.com/bazelbuild/bazel/tree/aragos-test-macos-default-platform and started a buildkite run with that at https://buildkite.com/bazel/bazel-at-head-plus-downstream/builds/735. This may take a while, let's see how it goes. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

aragos · 2019-01-08T22:05:48Z

Downstream tests have passed. I'm currently working on preserving the old default internally then I'll merge the PR.

keith · 2019-01-09T21:55:32Z

🎉 thanks a ton everyone!

keith requested a review from lberki as a code owner October 11, 2018 21:43

googlebot added the cla: yes label Oct 11, 2018

keith mentioned this pull request Oct 11, 2018

Running swift_test on macOS bazelbuild/rules_swift#51

Closed

irengrig assigned lberki Oct 12, 2018

irengrig added the WIP label Oct 12, 2018

lberki requested review from sergiocampama and removed request for lberki October 12, 2018 12:45

lberki assigned sergiocampama and unassigned lberki Oct 12, 2018

kangnak mentioned this pull request Nov 21, 2018

Build rule for compiling objc binaries for Mac OS X #125

Closed

jin added the z-team-Apple Deprecated. Send to rules_apple, or label team-Rules-CPP + platform:apple label Dec 10, 2018

keith mentioned this pull request Dec 11, 2018

Pass macOS version min flag in osx CROSSTOOL #6891

Closed

keith force-pushed the ks/default-platform-type branch from ef76e0c to 34029f4 Compare January 4, 2019 19:39

keith commented Jan 4, 2019

View reviewed changes

keith added 2 commits January 7, 2019 13:33

Change default apple_platform_type to macOS

7c4d51b

When using rules for Apple platforms that aren't platform specific, such as `swift_binary`, the default of iOS doesn't make sense, instead we should default this to the current platform, which currently can only be macOS.

Add --apple_platform_type=ios to some tests

bb70b73

keith force-pushed the ks/default-platform-type branch from 34029f4 to f2fd7c9 Compare January 7, 2019 21:45

keith requested review from allevato, DavidGoldman and dmaclach as code owners January 7, 2019 21:45

Fix more platform dependent tests

7a24b2d

keith force-pushed the ks/default-platform-type branch from f2fd7c9 to 7a24b2d Compare January 7, 2019 23:13

bazel-io closed this in f70d266 Jan 9, 2019

keith deleted the ks/default-platform-type branch January 9, 2019 21:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change default apple_platform_type to macOS #6366

Change default apple_platform_type to macOS #6366

keith commented Oct 11, 2018

allevato commented Oct 11, 2018

keith commented Oct 11, 2018

allevato commented Oct 11, 2018

lberki commented Oct 12, 2018

keith commented Oct 12, 2018

ulfjack commented Oct 15, 2018

sergiocampama commented Oct 15, 2018

keith commented Oct 15, 2018

keith commented Nov 20, 2018

sergiocampama commented Jan 4, 2019

keith commented Jan 4, 2019

keith commented Jan 4, 2019

keith Jan 4, 2019

aragos commented Jan 7, 2019 •

edited

Loading

keith commented Jan 7, 2019

aragos commented Jan 7, 2019

keith commented Jan 7, 2019

keith commented Jan 7, 2019

keith commented Jan 8, 2019

aragos commented Jan 8, 2019

keith commented Jan 8, 2019 via email

aragos commented Jan 8, 2019

keith commented Jan 9, 2019

Change default apple_platform_type to macOS #6366

Change default apple_platform_type to macOS #6366

Conversation

keith commented Oct 11, 2018

allevato commented Oct 11, 2018

keith commented Oct 11, 2018

allevato commented Oct 11, 2018

lberki commented Oct 12, 2018

keith commented Oct 12, 2018

ulfjack commented Oct 15, 2018

sergiocampama commented Oct 15, 2018

keith commented Oct 15, 2018

keith commented Nov 20, 2018

sergiocampama commented Jan 4, 2019

keith commented Jan 4, 2019

keith commented Jan 4, 2019

keith Jan 4, 2019

Choose a reason for hiding this comment

aragos commented Jan 7, 2019 • edited Loading

keith commented Jan 7, 2019

aragos commented Jan 7, 2019

keith commented Jan 7, 2019

keith commented Jan 7, 2019

keith commented Jan 8, 2019

aragos commented Jan 8, 2019

keith commented Jan 8, 2019 via email

aragos commented Jan 8, 2019

keith commented Jan 9, 2019

aragos commented Jan 7, 2019 •

edited

Loading