bazel icon indicating copy to clipboard operation
bazel copied to clipboard

Using `--cpu=darwin_x86_64` in `platform_mappings` triggers unnecessary toolchain evaluation

Open philsc opened this issue 2 years ago • 15 comments

Description of the bug:

This is a follow-up to #12897. The fix (#14096) doesn't address the original problem when platform_mappings is in use. When not using platform_mappings, there is no problem.

What's the simplest, easiest way to reproduce this bug? Please provide a minimal example if possible.

The objective is to prevent bazel from analysing objc_library targets on Linux.

Create this BUILD file:

objc_library(
    name = "objc",
    target_compatible_with = [
        "@platforms//os:macos",
    ],
)

platform(
    name = "macos",
    constraint_values = [
        "@platforms//os:macos",
    ],
)

The target is appropriately skipped on a Linux machine:

$ bazel build //:all
INFO: Analyzed target //:objc (0 packages loaded, 117 targets configured).
INFO: Found 1 target...
Target //:objc was skipped
INFO: Elapsed time: 0.481s, Critical Path: 0.04s
INFO: 1 process: 1 internal.
INFO: Build completed successfully, 1 total action

Now add a platform_mappings file:

platforms:
  //:macos
    --cpu=darwin_x86_64

flags:
  --cpu=darwin_x86_64
    //:macos

and you see the same error originally reported in #12897:

$ bazel build --platform_mappings=platform_mappings //:all
ERROR: /bazel-cache/phil/bazel/_bazel_phil/8299a41ccdd1478ccc4dfad80faab3e8/external/local_config_cc/BUILD:47: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_x86_64'
ERROR: /bazel-cache/phil/bazel/_bazel_phil/8299a41ccdd1478ccc4dfad80faab3e8/external/local_config_cc/BUILD:47:19: Analysis of target '@local_config_cc//:toolchain' failed
ERROR: Analysis of target '//:objc' failed; build aborted: 
INFO: Elapsed time: 0.232s
INFO: 0 processes.
FAILED: Build did NOT complete successfully (0 packages loaded, 258 targets configured)

It's worth noting that this only happens with --cpu=darwin_x86_64. If I instead add a --cpu=k8 platform mapping, then the error goes away again.

Which operating system are you running Bazel on?

x86_64 Debian 11

What is the output of bazel info release?

development version

If bazel info release returns development version or (@non-git), tell us how you built Bazel.

$ git log --oneline -1
61c433e896 (HEAD, upstream/master) Content clarifications and markdown fixes for consistency with other tutorials.
$ git status
HEAD detached at upstream/master
nothing to commit, working tree clean
$ bazel build -c opt //src:bazel-bin

What's the output of git remote get-url origin; git rev-parse master; git rev-parse HEAD ?

$ git remote get-url upstream
https://github.com/bazelbuild/bazel
$ git rev-parse HEAD
61c433e8968549781664194b9c046335f125c07a

Have you found anything relevant by searching the web?

N/A

Any other information, logs, or outputs that you want to share?

I will investigate possible solutions to this.

philsc avatar Aug 14 '22 01:08 philsc

As per @pcjanzen's comment, introducing the platform mapping for --cpu=darwin_x86_64 does indeed introduce a transition to //:macos. This means the //:objc target is not incompatible and gets evaluated.

Still trying to figure out what is going on exactly (and why).

philsc avatar Aug 14 '22 03:08 philsc

@gregestren , @katre , as always, ideas are welcome :grin:

philsc avatar Aug 14 '22 03:08 philsc

I'm unsure what the root problem is here.

i.e my quick reading is that a platform mapping sets the target to a Mac platform. So that's telling Bazel it should build the target for the Mac platform, which it would now expect to work?

Does your machine have no support for building for Mac at all? Even if you set a Mac --platforms at the command line?

gregestren avatar Aug 15 '22 18:08 gregestren

The goal is to disable objc_library targets from getting analyzed on Linux. Or at least not make bazel error out when running on Linux.

It's possible that I'm misunderstanding how platform mappings work. My assumption was that the mapping is intended to translate between --cpu and --platforms options. But it also appears to cause bazel to actually evaluate the objc_library target under the //:macos platform. Which is baffling to me since I never asked for that on the command line.

philsc avatar Aug 15 '22 19:08 philsc

The use case being discussed here is:

  • A multi-platform repository, containing some apple targets (e.g., objc_library), and
  • A platform_mappings file that is used to cause platform-specific constraints to be used in apple builds, and
  • A desire to skip building those apple targets when building for non-apple platforms by using target_compatible_with

I.e., we want to log into a linux box, type bazel build --cpu=k8 --platforms=//:linux //..., and have the objc_library targets be skipped.

Currently, they will not be skipped, because AppleCrosstoolTransition unconditionally changes the --cpu of objc_library targets from k8 to darwin_x86_64, and then platform_mappings changes the platform to one containing the @platforms//os:macos constraint.

The target configuration passed on the command line is effectively ignored; the target will always be compatible.

pcjanzen avatar Aug 15 '22 19:08 pcjanzen

Yes, that makes sense @pcjanzen (and I hope that addresses your confusion @philsc about why objc_library targets a different platform than you intended - it's set in a transition).

I understand your use case. Thanks for detailing it out. The challenge with this bug is that all pieces of functionality are working as intended. It's not a bug, per se. But you're showing how that can still produce a bad user experience.

Core configurability devs will review this this week as part of routine bug triage. Let's get another update from that review.

gregestren avatar Aug 15 '22 19:08 gregestren

Perhaps a related question is: What's the motivation behind objc_library performing this transition? Seems like it's making a decision that users should be making. I.e. I would imagine, all use cases of objc_library can still be satisfied without this self-transition.

philsc avatar Aug 15 '22 20:08 philsc

I can't remember, but it might have been - awkwardly enough - so bazel build //:all works when the package includes objc_library targets? i.e. there's no instance where building objc_library makes sense for non-Apple platforms so ensure by definition it's always building for an Apple platform.

@oquenchil do you remember?

gregestren avatar Aug 15 '22 20:08 gregestren

I can see the appeal of that logic before Incompatible Target Skipping. But now with Incompatible Target Skipping I would like to figure out what it would take to get rid of that behaviour. Or at least work around. I appreciate you looking at this during the next bug triage. Thanks!

Aside: If I may be so bold, were I to propose a newer version of objc_library written today, I would remove the self-transition and instead let users use something like platform_transition_filegroup to transition whole objc_binary targets. I.e. in "modern bazel" I don't see an advantage for individual libraries to introduce self-transitions. If a user wants to cross-compile an objc_library directly, they should ideally be able to set --platforms to a Mac platform.

philsc avatar Aug 16 '22 04:08 philsc

It would feel natural to me if every objc_library would automatically behave as if it had @platforms//os:macos in target_compatible_with instead of transitioning to that platform. That's possible today by making it a macro, but a provider-based solution may be cleaner.

fmeum avatar Aug 16 '22 06:08 fmeum

@gregestren (or maybe @katre , @googlewalt ?), if you could help me find the owner of the objc_* rules for their thoughts, I'd appreciate it.

I don't really know the objc_* rule set, so I don't have much to go on, but my naive understanding leads me to the following solutions:

  1. Drop the self-transition for objc_* rules.
  2. Make the objc_* rules use platforms natively and also drop the self transition.

Presumably either of these would require a --incompatible_* flag. There are probably solutions I'm not thinking of here.

Either way, I'm interested in writing a proposal and/or implementing it depending on what the owners think is the best course of action.

philsc avatar Aug 28 '22 06:08 philsc

Thank you for being patience. We have forwarded to right team and owner for this issue to address.

sgowroji avatar Aug 28 '22 11:08 sgowroji

@oquenchil - This thread is questioning the logic for objc_library to self-transition vs. declare Mac-compatibility and just be skipped when built for non-Apple environments. Can you shed light on the design concerns? Should objc_library built as a top-level target even on Mac environments? Where's the current GitHub code for it?

gregestren avatar Sep 14 '22 21:09 gregestren

@googlewalt would you know the answer to Greg's question?

oquenchil avatar Sep 16 '22 12:09 oquenchil

I think we are all in agreement that having a transition in objc_library was a mistake. My understanding was that it was put in so that when a linux user builds an objc_library target interactively, bazel would do something useful instead of failing. The transition has caused issues from time to time and we would like to eventually get rid of it -- but it hasn't bubbled up to the top of the priority list yet. We don't yet know the scope of the cleanup that would be required; the first step would be to implement it behind an incompatible flag.

For now, one "gross" hack that might work for your target_comatible_with project is to add --macos_cpus=bogus to your linux build flags. The flag shouldn't affect anything built on linux, but it would stop objc_library from transitioning the build to a platform you don't want.

googlewalt avatar Sep 21 '22 20:09 googlewalt

FYI @googlewalt 's team recently did some experiments on cleaning up the objc_library transition.

gregestren avatar Nov 02 '22 23:11 gregestren

At the risk of stating the obvious, this is a problem inherent to any rule that self-transitions on --platform or --cpu, not just objc_library.

pcjanzen avatar Nov 03 '22 03:11 pcjanzen

as far as removing that transition I've pulled that into a separate issue here https://github.com/bazelbuild/bazel/issues/16870

keith avatar Nov 28 '22 20:11 keith

The objc_library transition is now disabled pending deletion

keith avatar Dec 18 '23 18:12 keith