Cannot use target_compatible_with to ignore MacOS only targets

[jagobagascon]

Description of the problem:

I'm trying to use the target_compatible_with attribute to automatically skip MacOS only targets on other platforms, but I just get an error every time. I have a simple objc_library rule with target_compatible_with = [ "@platforms//os:macos" ] but whenever I run bazel build //...:all on non MacOS platforms I get this:

ERROR: /root/.cache/bazel/_bazel_root/eab0d61a99b6696edb3d2aff87b585e8/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: Analysis of target '//ble/platform/macos:ble_macos' failed; build aborted: Analysis of target '@local_config_cc//:toolchain' failed

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

Any objc_library rule fails instead of being ignored.

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

I added a simple example here: jagobagascon/bazel-target-compatible
It's just an empty objc_library rule but you can se how the Ubuntu build fails while the MacOS just works: jagobagascon/bazel-target-compatible/actions/runs/512144112

What operating system are you running Bazel on?

I'm running it on a Docker image based on ubuntu:focal-20200423, but it also happens on a GitHub-hosted ubuntu-18.04.

What's the output of bazel info release?

release 4.0.0

[katre]

I can reproduce this: it looks like objc_library has a self-transition (AppleCrosstoolTransition), which changes the value of the --cpu flag to one that's appropriate for building objc code. Unfortunately, this causes two issues:

1. You don't have a cc toolchain for the selected cpu value (darwin_x86_64, looks like), which causes an analysis failure.
2. This failure happens before the incompatible target skipping code runs.

I'm passing this off the @gregestren to see if there's a way to restructure the target skipping to ignore analysis failures in dependencies, but I'm not sure this is possible.

[gregestren]

I think it's reasonable to calculate target incompatibility before its deps are analyzed (at least direct incompatibility).

But we need to know the current platform, which generally comes through toolchain resolution. Does the current toolchain resolution logic provide that before trying to find (non-existent) toolchains? i.e. the unresolved computation produces platform info?

[katre]

The target platform is loaded before toolchains are actually resolved, but I'm not sure there's a good way to pass that information back out to ConfiguredTargetFunction along with the resolution error.

There are a lot of arguments for making all toolchains optional (and letting rules themselves throw errors if they need a toolchain that doesn't exist), and this seems to be one of them.

[gregestren]

Do you think ConfiguredTargetFunction could just call

bazel/src/main/java/com/google/devtools/build/lib/skyframe/PlatformLookupUtil.java

Line 50 in 8346ea4

public static Map<ConfiguredTargetKey, PlatformInfo> getPlatformInfo(

directly, constructing that platform key with

bazel/src/main/java/com/google/devtools/build/lib/skyframe/ToolchainResolutionFunction.java

Lines 212 to 216 in 8346ea4

	ConfiguredTargetKey targetPlatformKey =
	ConfiguredTargetKey.builder()
	.setLabel(targetPlatformLabel)
	.setConfiguration(configuration)
	.build();

?

If so, the other challenge would be to short-circuit return a ConfiguredTarget hard-set to IncompatiblePlatformProvider. I think that'd be straightforward, albeit it complicates ConfiguredTargetFunction logic even more.

Re: generalizing optional toolchains, @katre offered to write up deeper thoughts on that.

CC @philsc for FYI, or if I'm getting anything wrong.

[katre]

I think that would work to get a target platform. I can't say about the logic for the incompatible platform provider.

[gregestren]

To be clear, I'd really like to prototype this but I don't have time at the moment. I am keeping this issue in my back burner memory - I know it's important to work out first bugs on a new feature to really make the feature land well.

[philsc]

I'll take a stab at your proposed idea this weekend and report back with what I find. It sounds reasonable, but also sounds like it could mean moving the entire logic to a completely different spot.

[gregestren]

Awesome, thanks! I think if injected carefully the change won't be overly invasive. But yeah it's a bit delicate to nudge all the parts exactly right.

[philsc]

I spent a couple of hours on this without much progress. I think constructing the PlatformInfo is relatively straightforward.

However, what I'm struggling with is how to compare that against the target_compatible_with attribute. How do I access the configured attributes? Can I even get access to the configured attributes before the original toolchain error happens?

[gregestren]

This might call for some virtual pair programming. Could you share with me a branch with your code? I'd also need to look down at the code level to sort through this stuff.

[philsc]

Here are my hacks from last weekend: https://github.com/philsc/bazel/tree/unreviewed/objc_library

Specifically, I think this is the last place where we can evaluate incompatibility:
https://github.com/philsc/bazel/blob/8f78bbf41a5a7eb17141f36d26b647350bcc425c/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java#L326

(Or we could add something inside of computeDependencies(), but I'm pretty sure we never get after computeDependencies() because in that callchain is where the toolchain error happens)

i had trouble going from target (and the other available information) to something that lets me access the configured attributes.

[glerchundi]

Hi 👋! Running into this issue as well, is there any progress on this? Thanks!!

[gregestren]

@philsc were you waiting for followup from me?

[philsc]

@philsc were you waiting for followup from me?

Not really, no. I've just been extremely lazy in regards to bazel these past few months :P
I can take another look this weekend.

[philsc]

Apologies for the delay. Last weekend was too busy, but this weekend I'm planning to look at this again.

[glerchundi]

Thanks! 🙏

[philsc]

I'm starting to remember why I didn't solve this the last time I looked into it.
I need to understand this part of the code better in order to make changes here. The guidance @gregestren has provided helps, but I still need to understand this code better. It's currently still a bit "magic".
In other words, I feel like I'm making progress, but it's slow going. At the moment it's an archeology trip.

[philsc]

I feel like I'm close.

@gregestren , I need a way to convert the Labels from ConfiguredAttributeMapper to either ConstraintValueInfo instances or <? extends ProviderCollection> instances. See the MAGIC note in my current hacky commit:
https://github.com/philsc/bazel/blob/ab10d8621c79b0c22329af3674a1de2566392129/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java#L336-L361

In RuleContextConstraintSemantics we could use ruleContext.getPrerequisites("target_compatible_with") to get a provider collection, but I don't think it's an option here.

Any thoughts you have would be much appreciated!

[philsc]

Actually, it looks like the following code works to determine the equivalent TransitiveInfoCollection targets:
https://github.com/philsc/bazel/blob/66091e57c46540400d354172adf1c01bb27de994/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java#L346-L401

So I think we have a way to detect direct incompatibility. Now we just need to call RuleContextConstraintSemantics.createIncompatibleConfiguredTarget() but without a ruleContext. Sounds plausible/doable, but I haven't looked at that yet.

[gregestren]

That all looks about right to me.

I'd even bypass resolveConfiguredTargetDependencies, which has a bunch of extra logic you don't really need for this, and try going directly for calling another ConfiguredTargetFunction for the dep. For example, this pattern:

bazel/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java

Lines 848 to 854 in b3aacbb

	Iterable<SkyKey> depKeys =
	Iterables.concat(
	Iterables.transform(deps, Dependency::getConfiguredTargetKey),
	Iterables.transform(
	deps, input -> PackageValue.key(input.getLabel().getPackageIdentifier())));
	Map<SkyKey, ValueOrException<ConfiguredValueCreationException>> depValuesOrExceptions =
	env.getValuesOrThrow(depKeys, ConfiguredValueCreationException.class);

In short, construct a ConfiguredTargetKey with the current configuration and the new label, call env.getValue() on it, and the result should directly give you a ConfiguredTargetAndData from which you can get a TransitiveInfoCollection.

I think that would produce slightly less code (although I could be wrong).

[gregestren]

Here's another example of translating ConfiguredTargetKeys into ConfiguredTargetAndDatas:

bazel/src/main/java/com/google/devtools/build/lib/skyframe/PlatformLookupUtil.java

Line 68 in e362fc9

PlatformInfo platformInfo = findPlatformInfo(key, values.get(key));

bazel/src/main/java/com/google/devtools/build/lib/skyframe/PlatformLookupUtil.java

Lines 142 to 147 in e362fc9

	ConfiguredTargetValue ctv = (ConfiguredTargetValue) valueOrException.get();
	if (ctv == null) {
	return null;
	}
	ConfiguredTarget configuredTarget = ctv.getConfiguredTarget();

That example further casts into PlatformInfo, which is more work than you need to do. But the core logic is following the same sequence.

[philsc]

Sorry for dropping the ball on this ticket. It's been a busy few months. I'm resuming work on this at the end of August.

If anyone else wants to take it over in the meantime, I can totally understand that.

[philsc]

I updated my proof-of-concept at https://github.com/philsc/bazel/tree/unreviewed/objc_library.
It now passes all the existing tests and properly skips objc_library targets.
However, it's very hacky.
I need to spend some more time cleaning it up and incorporating all the ideas that @gregestren provided earlier.
(also, it's based on a ~5 month old version of master)

[philsc]

I created a draft PR after rebasing on latest master. Still need to actually incorporate Greg's feedback.

[gregestren]

Thanks for the update! Happy to provide any more feedback as needed!

[philsc]

Note to self: test how this refactor behaves with select() against the toolchain:

config_setting(
    name = "compiler_gcc",
    flag_values = {
        "@bazel_tools//tools/cpp:compiler": "gcc",
    },
)

cc_library(
    name = "gcc-only-lib",
    target_compatible_with = select({
        ":compiler_gcc": [],
        "//conditions:default": ["@platforms//:incompatible"],
    }),
    ...
)

[gregestren]

Added @philsc as co-assignee since #14096 should cover this.

[pcjanzen]

Note that the refactoring done in 72787a1 doesn't solve this problem in all cases. Consider

==> BUILD <==
objc_library(
    name = 'ble',
    target_compatible_with = [
        "@platforms//os:macos",
    ],
)

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

==> platform_mappings <==
platforms:
  //:macos
    --cpu=darwin_x86_64

flags:
  --cpu=darwin_x86_64
    //:macos

In this case, objc_library's self-transition on --cpu leads to a transition on --platforms, and then ble's target platform is actually target_compatible_with macos again (regardless of the value for --platforms passed on the command line). So a toolchain is once again required.

[philsc]

Ah dang. Looks like the platform_mappings file is still triggering this issue. I will think about possible solutions.

[philsc]

Filed #16099 as a follow-up.