Define cc-compiler-darwin in Xcode toolchain

[fmeum]

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

[fmeum]

cc @keith

[keith]

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

[fmeum]

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]

Hi Keith,

Does it still make sense to merge this?

[fmeum]

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

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]

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

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]

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]

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]

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]

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

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]

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

[fmeum]

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]

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]

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

[fmeum]

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

@keith Who would be responsible for importing and merging?

[keith]

@oquenchil

[fmeum]

@googlewalt Friendly ping

[fmeum]

@bazel-io flag

[fmeum]

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

@bazel-io fork 5.3.0