macOS: respect DEVELOPER_DIR for Xcode selection.

[benjivos]

On macOS, this change ensures that when DEVELOPER_DIR environment variable is used, it will be propagated to /usr/bin/xcrun to query the SDKROOT. This takes precedence over the --xcode_version flag when selecting the Xcode installation. This aligns with the intended use as documented in man xcrun:

The tool xcode-select(1) is used to set a system default for the active developer directory, and may be overridden by the DEVELOPER_DIR environment variable (see ENVIRONMENT).

To leverage this, developers must propagate the DEVELOPER_DIR environment variable to Bazel actions using --action_env=DEVELOPER_DIR.

[keith]

what's the motivation here to take precedence over xcode_version? in today's world the intent of the logic is that bazel does a lookup once to discover the current Xcode version (and that could take DEVELOPER_DIR into account) and then after that all invocations use that discover. The weird part about this is that separate actions could technically use different Xcode versions, for example in your example --action_env wouldn't be enough you'd also need --host_action_env

[benjivos]

The motivation is to ensure that when DEVELOPER_DIR is explicitly passed to actions via --action_env or --host_action_env, those actions consistently use the specified Xcode. While the current implementation attempts to respect DEVELOPER_DIR, it can lead to inconsistencies where a single action uses paths from different Xcode installations. This is because the SDKROOT is derived from the xcode-select -p, and other paths from the DEVELOPER_DIR environment variable.

My understanding is that the intent should be that if a user explicitly provides DEVELOPER_DIR for an action, that action should use that Xcode. The current implementation doesn't guarantee this.

To illustrate the problem, consider this scenario:

• The system default Xcode is set to /Applications/Xcode.app (using xcode-select -s /Applications/Xcode.app).
• We want to run a Bazel action using /Applications/Xcode2.app.
• We pass DEVELOPER_DIR=/Applications/Xcode2.app using --action_env and --host_action_env.

The current implementation can result in an action that uses paths from both Xcode installations. Here's a concrete example, runnable with the attached example.zip file:

DEVELOPER_DIR=/Applications/Xcode2.app  bazel run -s --action_env=__WRAPPED_CLANG_LOG_ONLY=1 --action_env=DEVELOPER_DIR --host_action_env=DEVELOPER_DIR //:hello

This command will print the clang invocation, which includes both /Applications/Xcode.app and /Applications/Xcode2.app:

/usr/bin/xcrun clang -D_FORTIFY_SOURCE=1 -fstack-protector -fcolor-diagnostics -Wall -Wthread-safety -Wself-assign -fno-omit-frame-pointer -O0 -DDEBUG -fdebug-prefix-map=/private/tmp/bazel-sandbox.6cafcf99323a62b5375265dd657c2b4578a1b7be1607e50d8be25dad19416f92/darwin-sandbox/7/execroot/_main=. -fdebug-prefix-map=/Applications/Xcode2.app=/PLACEHOLDER_DEVELOPER_DIR -iquote . -iquote bazel-out/darwin_arm64-fastbuild/bin -iquote external/bazel_tools -iquote bazel-out/darwin_arm64-fastbuild/bin/external/bazel_tools -MD -MF bazel-out/darwin_arm64-fastbuild/bin/_objs/hello/hello.d -frandom-seed=bazel-out/darwin_arm64-fastbuild/bin/_objs/hello/hello.o -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX15.1.sdk -F/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX15.1.sdk/System/Library/Frameworks -F/Applications/Xcode2.app/Platforms/MacOSX.platform/Developer/Library/Frameworks -no-canonical-prefixes -pthread -no-canonical-prefixes -Wno-builtin-macro-redefined -D__DATE__="redacted" -D__TIMESTAMP__="redacted" -D__TIME__="redacted" -target arm64-apple-macosx15.1 -c hello.c -o bazel-out/darwin_arm64-fastbuild/bin/_objs/hello/hello.o

My change ensures that, in this scenario, only /Applications/Xcode2.app will be used for the entire action, providing the expected behavior when DEVELOPER_DIR is explicitly specified. The xcode_version setting would still be used if DEVELOPER_DIR isn't explicitly passed via --action_env or --host_action_env.

[keith]

Right the intent in bazel instead today is that you never pass DEVELOPER_DIR to actions, and instead bazel handles passing that itself, which it does by determining the DEVELOPER_DIR when it starts to discover xcode versions (if that part isn't working we should fix that)

the issue with changing this is that theoretically those could mismatch, and then things could end up with 2 values in other ways.

put another way we could make bazel reject ever setting DEVELOPER_DIR / SDKROOT in action_env, since that should be a bazel implementation detail.

[keith]

I was missing a piece in order to fully understand this. The key is that if you have 2 different copies of the same Xcode version, and xcode-locator cannot differentiate.

I think we should land but clearly not recommend for folks, since most workflows should go through the normal discovery paths and then get DEVLEOPER_DIR set automatically.

the alternative to this would be to make xcode_autoconf take the DEVELOPER_DIR more literally, where today it only uses it to get the version and then look that up based on xcode-locator, but I think that path probably isn't worth it for this edge case.

[benjivos]

@allevato would you be able to have a look at this PR? Thank you!

[allevato]

The key is that if you have 2 different copies of the same Xcode version, and xcode-locator cannot differentiate.

This doesn't seem like a compelling case to support. Why do you intentionally have two copies of the same version? Or is the issue that you somehow have a duplicate and it's not getting distinguished? If the latter, that's an argument to improve xcode-locator.

Otherwise, I'm very much against leaking external state (even intentionally via --action_env-like flags) into the Xcode selection process. There's already too much complexity there and this adds to it.

[allevato]

(I'm not actually requesting changes here, I just want to have signal that I don't think we should do this)

[allevato]

Here's a direction to explore that might be viable (I haven't thought it all the way through, though).

There shouldn't be a lot of places in Bazel's Java core where the --xcode_version flag is required to be a dotted version number. XcodeLocalEnvProvider.java converts it to one, but then it turns around and converts it right back to a string to pass it to xcode-locator.

So what if you could just pass the developer dir of the Xcode you want to use as that flag? --xcode_version=/Applications/Xcode2.app/Contents/Developer. That way, if the value passed in that flag isn't a valid version number, we just assume it's the desired developer dir verbatim and don't even call xcode-locator to find it. That makes the choice very explicit instead of relying on propagation of an external environment variable.

I don't know if this would have any knock-on effects downstream, like with the xcode_config rules (either the built-in ones or the new ones in apple_support). But that's the direction I would prefer to explore.

[benjivos]

This doesn't seem like a compelling case to support. Why do you intentionally have two copies of the same version? Or is the issue that you somehow have a duplicate and it's not getting distinguished? If the latter, that's an argument to improve xcode-locator.

We actually have multiple copies installed of the same Xcode version with different local changes potentially, requiring us to unambiguously set DEVELOPER_DIR.

So what if you could just pass the developer dir of the Xcode you want to use as that flag? --xcode_version=/Applications/Xcode2.app/Contents/Developer. That way, if the value passed in that flag isn't a valid version number, we just assume it's the desired developer dir verbatim and don't even call xcode-locator to find it. That makes the choice very explicit instead of relying on propagation of an external environment variable.

Thanks for your direction, I also like the explicit nature of it more than my initial patch. I will try to update my change with your suggestions.

[allevato]

@susinmotion This seems fine to me but I'd appreciate a second pair of eyes on it.

[allevato]

Yep! If @susinmotion is good, I'm good.

[allevato]

I have no power to merge it, though.

[benjivos]

@allevato who can help merging it? Thank you!

[benjivos]

@iancha1992 could you help merging this change? Thank you!

[iancha1992]

@bazel-io fork 8.3.0