Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Using --cpu=darwin_x86_64 in platform_mappings triggers unnecessary toolchain evaluation #16099

Open
philsc opened this issue Aug 14, 2022 · 19 comments
Assignees
Labels
P2 We'll consider working on this in future. (Assignee optional) team-Rules-ObjC Issues for Objective-C maintainers type: bug

Comments

@philsc
Copy link
Contributor

philsc commented Aug 14, 2022

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 philsc changed the title Using platform_mappings triggers unnecessary toolchain evaluation Using --cpu=darwin_x86_64 in platform_mappings triggers unnecessary toolchain evaluation Aug 14, 2022
@philsc
Copy link
Contributor Author

philsc commented Aug 14, 2022

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
Copy link
Contributor Author

philsc commented Aug 14, 2022

@gregestren , @katre , as always, ideas are welcome 😁

@gregestren gregestren added team-Configurability platforms, toolchains, cquery, select(), config transitions type: bug untriaged labels Aug 15, 2022
@gregestren
Copy link
Contributor

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?

@philsc
Copy link
Contributor Author

philsc commented Aug 15, 2022

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.

@pcjanzen
Copy link
Contributor

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.

@gregestren
Copy link
Contributor

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.

@philsc
Copy link
Contributor Author

philsc commented Aug 15, 2022

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.

@gregestren
Copy link
Contributor

gregestren commented Aug 15, 2022

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?

@philsc
Copy link
Contributor Author

philsc commented Aug 16, 2022

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.

@fmeum
Copy link
Collaborator

fmeum commented Aug 16, 2022

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.

@philsc
Copy link
Contributor Author

philsc commented Aug 28, 2022

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

@sgowroji sgowroji added team-Rules-ObjC Issues for Objective-C maintainers and removed team-Configurability platforms, toolchains, cquery, select(), config transitions labels Aug 28, 2022
@sgowroji
Copy link
Member

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

@gregestren
Copy link
Contributor

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

@oquenchil
Copy link
Contributor

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

@googlewalt
Copy link
Contributor

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.

@gregestren
Copy link
Contributor

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

@pcjanzen
Copy link
Contributor

pcjanzen commented Nov 3, 2022

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.

@keith
Copy link
Member

keith commented Nov 28, 2022

as far as removing that transition I've pulled that into a separate issue here #16870

@keith keith added P2 We'll consider working on this in future. (Assignee optional) and removed untriaged labels Dec 18, 2023
@keith
Copy link
Member

keith commented Dec 18, 2023

The objc_library transition is now disabled pending deletion

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P2 We'll consider working on this in future. (Assignee optional) team-Rules-ObjC Issues for Objective-C maintainers type: bug
Projects
None yet
Development

No branches or pull requests

8 participants