Define cc-compiler-darwin in Xcode toolchain by fmeum · Pull Request #14796 · bazelbuild/bazel

fmeum · 2022-02-11T15:34:45Z

Previously, if the xcode_locator failed and cc_autoconf_toolchain used
the non-Xcode C++ toolchain as a fallback, its reference to
@local_config_cc//:cc-compiler-darwin, where darwin is the legacy cpu
value for x86_64 macOS, would be invalid.

Fixes #14459

This reverts commit 42f4f91. No longer needed as the root cause of the failures has likely been determined and will be fixed by bazelbuild/bazel#14796

fmeum · 2022-02-11T15:40:45Z

cc @keith

This reverts commit 42f4f91. No longer needed as the root cause of the failures has likely been determined and will be fixed by bazelbuild/bazel#14796

keith · 2022-02-11T17:33:19Z

looks like this is on the right track, but i guess one is still sneaking in

fmeum · 2022-02-11T18:12:55Z

looks like this is on the right track, but i guess one is still sneaking in

I pushed a new approach. The problem is that cc_autoconf doesn't know whether cc_autoconf_toolchain used the non-Xcode fallback or not, so an alias is a better way to ensure it always finds a cc-compiler-... target.

oquenchil · 2022-04-06T14:40:53Z

Hi Keith,

Does it still make sense to merge this?

fmeum · 2022-04-06T15:25:29Z

@keith I'm still seeing CI failures related to this:

ERROR: /Users/runner/work/rules_jni/rules_jni/tests/libjvm_stub/BUILD.bazel:102:11: Target '//libjvm_stub:libjvm_test_lib' depends on toolchain '@local_config_cc//:cc-compiler-darwin', which cannot be found: error loading package '@local_config_cc//': cannot load '@local_config_cc_toolchains//:osx_archs.bzl': no such file'
ERROR: Analysis of target '//libjvm_stub:libjvm_test_lib' failed; build aborted: Analysis failed

It's hard to verify that this PR would fix the issue, but it seems likely.

keith · 2022-04-06T17:33:56Z

I don't think this PR will fix that issue, but where is the bad CPU value sneaking in from? It seems like we might want to fix that or this could cause other issues?

fmeum · 2022-04-06T20:12:39Z

@keith If xcode_locator fails in cc_autoconf_toolchains on an Intel Mac, then it will fall back to _generate_cpp_only_build_file, which ends up using the toolchain in BUILD.toolchains.tpl. The CPU value darwin comes from here in that case. At the same time cc_autoconf will call configure_osx_toolchain, which uses darwin_x86_64 instead.

I remember running into some issues when changing the return value of get_cpu_name, but I can give that another try if you prefer that solution.

keith · 2022-04-06T20:16:55Z

will call configure_osx_toolchain, which uses darwin_x86_64 instead.

Where is this happening? The only cpu_value I see being passed around comes from get_cpu_name

keith · 2022-04-06T20:17:55Z

I remember running into some issues when changing the return value of get_cpu_name, but I can give that another try if you prefer that solution.

This does seem like it would be preferred since IMO darwin is now ambiguous and shouldn't be used, but I don't know how much that would cascade

fmeum · 2022-04-06T20:21:17Z

Where is this happening? The only cpu_value I see being passed around comes from get_cpu_name

Those are indeed the only CPU values passed around, but when cc_autoconf calls configure_osx_toolchain, the generated BUILD file is based on https://github.com/bazelbuild/bazel/blob/c9f9d7b70514c3a102c27b285dc6f94ed011e6a0/tools/osx/crosstool/BUILD.tpl, which does not contain a cc-compiler-darwin target, but only cc-compiler-darwin_x86_64 (see https://github.com/bazelbuild/bazel/blob/c9f9d7b70514c3a102c27b285dc6f94ed011e6a0/tools/osx/crosstool/osx_archs.bzl). Hence the idea of adding an alias.

keith · 2022-04-06T20:29:08Z

thanks for catching me up. I would be interested to see if you added x86_64 in the place you linked what the first issue you hit is so we could try to judge what else might be a problem

fmeum · 2022-04-07T08:17:37Z

@keith

Thu Apr  7 07:45:47 UTC 2022
** test_disable_cc_toolchain_detection *****************************************
-- Test log: -----------------------------------------------------------
$TEST_TMPDIR defined: output root default is '/private/var/tmp/_bazel_buildkite/9ad5a35d6b282ff26eac6daf10a2843a/sandbox/darwin-sandbox/5815/execroot/io_bazel/_tmp/3a63f12eb3b00bf266b2cf45eae7c788' and max_idle_secs default is '15'.
Loading: 
Loading: 0 packages loaded
Analyzing: target @local_config_cc//:toolchain (1 packages loaded, 0 targets configured)
ERROR: /private/var/tmp/_bazel_buildkite/9ad5a35d6b282ff26eac6daf10a2843a/sandbox/darwin-sandbox/5815/execroot/io_bazel/_tmp/3a63f12eb3b00bf266b2cf45eae7c788/root/959814f456f0291bda5c15386f30c80f/external/local_config_cc/BUILD:28:19: in cc_toolchain_suite rule @local_config_cc//:toolchain: cc_toolchain_suite '@local_config_cc//:toolchain' does not contain a toolchain for cpu 'darwin'
ERROR: /private/var/tmp/_bazel_buildkite/9ad5a35d6b282ff26eac6daf10a2843a/sandbox/darwin-sandbox/5815/execroot/io_bazel/_tmp/3a63f12eb3b00bf266b2cf45eae7c788/root/959814f456f0291bda5c15386f30c80f/external/local_config_cc/BUILD:28:19: Analysis of target '@local_config_cc//:toolchain' failed
ERROR: Analysis of target '@local_config_cc//:toolchain' failed; build aborted: 
INFO: Elapsed time: 0.189s
INFO: 0 processes.
FAILED: Build did NOT complete successfully (7 packages loaded, 19 targets configured)
FAILED: Build did NOT complete successfully (7 packages loaded, 19 targets configured)
------------------------------------------------------------------------
test_disable_cc_toolchain_detection FAILED: Fake toolchain target causes analysis errors.
/private/var/tmp/_bazel_buildkite/9ad5a35d6b282ff26eac6daf10a2843a/sandbox/darwin-sandbox/5815/execroot/io_bazel/bazel-out/darwin-fastbuild/bin/src/test/shell/bazel/cc_integration_test.runfiles/io_bazel/src/test/shell/bazel/cc_integration_test:1007: in call to test_disable_cc_toolchain_detection

Tear down:

INFO[cc_integration_test 2022-04-07 07:45:47 (+0000)] Cleaning up workspace

�[0;31mFAILED�[0m: test_disable_cc_toolchain_detection

fmeum · 2022-04-07T09:14:28Z

The "empty" toolchain at

bazel/tools/cpp/cc_configure.bzl

Line 103 in bbf34eb

"%{cpu}": get_cpu_value(repository_ctx),

still relies on the legacy CPU value darwin. I could introduce a parameter for get_cpu_value that would conditionally return the legacy CPU value, but that seems worse to me.

keith · 2022-04-11T21:58:11Z

Where's the empty case defined? Since this line you linked would return your new value correctly right?

fmeum · 2022-04-12T06:17:34Z

Where's the empty case defined? Since this line you linked would return your new value correctly right?

It returns the new one and emits it into

bazel/tools/cpp/BUILD.empty.tpl

Line 31 in bbf34eb

"%{cpu}|local": ":local",

But that test seems to exercise the legacy toolchain resolution, which then looks for a match for the legacy CPU value darwin (the value of --cpu as determined by AutoCpuConverter) and can't find one as the only available cpu type is darwin_x86_64. Changing the return value of AutoCpuConverter to match darwin_x86_64 would probably break toolchain resolution for anybody who isn't on platforms yet.

keith · 2022-04-14T00:01:33Z

That change was attempted in the past and reverted #12918

I'm not sure if there's a good enough reason to try to ship something like that again. Realistically folks shouldn't use darwin as a CPU but it'll probably take time for people to fix the various select()s that rely on that.

keith · 2022-04-14T00:03:42Z

well sorry for all the back and forth just to get to this. I think we should commit your original alias for now

Previously, if the xcode_locator failed and cc_autoconf_toolchain used the non-Xcode C++ toolchain as a fallback, its reference to @local_config_cc//:cc-compiler-darwin, where darwin is the legacy cpu value for x86_64 macOS, would be invalid.

fmeum · 2022-04-14T04:56:51Z

@keith No worries, thanks for taking the time to go through the possible alternatives with me. I'm also interested in merging something that will stick and doesn't complicate the situation unnecessarily.

I force pushed the alias version of the PR.

fmeum · 2022-05-04T15:57:22Z

@keith Who would be responsible for importing and merging?

keith · 2022-05-04T16:04:43Z

@oquenchil

fmeum · 2022-05-25T12:19:05Z

@googlewalt Friendly ping

fmeum · 2022-06-03T12:33:24Z

@bazel-io flag

fmeum · 2022-06-03T12:34:30Z

@ckolli5 Could this still be cherry-picked into 5.2.0? It's a very small change and users run into the issue frequently.

ckolli5 · 2022-06-17T15:18:09Z

@bazel-io fork 5.3.0

Previously, if the xcode_locator failed and cc_autoconf_toolchain used the non-Xcode C++ toolchain as a fallback, its reference to `@local_config_cc//:cc-compiler-darwin`, where darwin is the legacy cpu value for x86_64 macOS, would be invalid. Fixes #14459 Closes #14796. PiperOrigin-RevId: 451860477 Change-Id: Iec115f600ebb7ac0786b2169276d25e3ff5d54bf Co-authored-by: Fabian Meumertzheim <[email protected]>

fmeum mentioned this pull request Feb 11, 2022

High rate of spurious CI failures on macOS machines #14459

Closed

fmeum mentioned this pull request Feb 11, 2022

Revert "Print local_config_cc/BUILD in macOS CI" fmeum/rules_jni#51

Merged

fmeum force-pushed the 14459-fix-darwin-cpu-value branch from 6a716d6 to de84788 Compare February 11, 2022 16:05

fmeum marked this pull request as draft February 11, 2022 16:53

fmeum force-pushed the 14459-fix-darwin-cpu-value branch 2 times, most recently from 537baa5 to d7c3dfd Compare February 11, 2022 18:10

fmeum marked this pull request as ready for review February 11, 2022 18:10

fmeum force-pushed the 14459-fix-darwin-cpu-value branch from d7c3dfd to f5ff0fe Compare February 11, 2022 18:12

fmeum force-pushed the 14459-fix-darwin-cpu-value branch from 5732d9a to f5ff0fe Compare February 12, 2022 10:42

fmeum changed the title ~~Emit correct darwin_x86_64 CPU value if not using Xcode~~ Define cc-compiler-darwin in Xcode toolchain Feb 12, 2022

oquenchil added team-Rules-ObjC Issues for Objective-C maintainers untriaged labels Feb 14, 2022

gregestren assigned oquenchil Mar 7, 2022

fmeum force-pushed the 14459-fix-darwin-cpu-value branch from f5ff0fe to aa54435 Compare April 7, 2022 07:15

fmeum force-pushed the 14459-fix-darwin-cpu-value branch from 35aef3c to 4fca2bf Compare April 14, 2022 04:55

keith approved these changes Apr 14, 2022

View reviewed changes

oquenchil assigned googlewalt and unassigned oquenchil May 5, 2022

oquenchil requested a review from googlewalt May 5, 2022 07:55

googlewalt approved these changes May 25, 2022

View reviewed changes

copybara-service Bot closed this in 2d42925 May 30, 2022

fmeum deleted the 14459-fix-darwin-cpu-value branch June 3, 2022 12:33

bazel-io added the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label Jun 3, 2022

bazel-io removed the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label Jun 17, 2022

bazel-io mentioned this pull request Jun 17, 2022

[5.3.0] Define cc-compiler-darwin in Xcode toolchain #15693

Closed

ckolli5 mentioned this pull request Jun 20, 2022

Define cc-compiler-darwin in Xcode toolchain #15709

Merged

Conversation

fmeum commented Feb 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fmeum commented Feb 11, 2022

Uh oh!

keith commented Feb 11, 2022

Uh oh!

fmeum commented Feb 11, 2022

Uh oh!

oquenchil commented Apr 6, 2022

Uh oh!

fmeum commented Apr 6, 2022

Uh oh!

keith commented Apr 6, 2022

Uh oh!

fmeum commented Apr 6, 2022

Uh oh!

keith commented Apr 6, 2022

Uh oh!

keith commented Apr 6, 2022

Uh oh!

fmeum commented Apr 6, 2022

Uh oh!

keith commented Apr 6, 2022

Uh oh!

fmeum commented Apr 7, 2022

Uh oh!

fmeum commented Apr 7, 2022

Uh oh!

keith commented Apr 11, 2022

Uh oh!

fmeum commented Apr 12, 2022

Uh oh!

keith commented Apr 14, 2022

Uh oh!

keith commented Apr 14, 2022

Uh oh!

fmeum commented Apr 14, 2022

Uh oh!

fmeum commented May 4, 2022

Uh oh!

keith commented May 4, 2022

Uh oh!

fmeum commented May 25, 2022

Uh oh!

fmeum commented Jun 3, 2022

Uh oh!

fmeum commented Jun 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ckolli5 commented Jun 17, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

fmeum commented Feb 11, 2022 •

edited

Loading

fmeum commented Jun 3, 2022 •

edited

Loading