Target-level modifiers are not applied when depending on an `attrs.dep()`

[cbarrete]

Problem and repro

Consider the following:

BUCK

cxx_binary(
    name = "bin",
    srcs = ["main.cpp"],
    modifiers = ["//build/platforms:release"],
)

command_alias(
    name = "cmd",
    exe = ":bin",
)

main.cpp

#include <iostream>

int main()
{
#ifdef NDEBUG
    std::cout << "release" << std::endl;
#else
    std::cout << "debug" << std::endl;
#endif
    return 0;
}

> buck2 run :bin
release
> buck2 run :cmd
debug

I would expect :bin to be built as a dependency of :cmd the same way as it is when built directly via the CLI, but it is not.

This is a simplistic example, but basically any modifiers set on targets in the build graph seem to be completely ignored.

I believe this to be a bug, because if a target requires some modifiers to be applied, the build may be completely broken, and buck2 run :cmd will result in a build failure, rather than just the wrong configuration being built.

Some of the things I have tried

Writing a simpler rule

I've written my own command rule, in case there was something weird about command_alias. There is not, I got the same behavior.

Using attrs.arg() or attrs.configured_dep()

Using attrs.arg() and using a string parameter macro to pass :bin (either via $(exe_target) or $(location)) made no difference.

attrs.configured_dep() does not really help, because while it can change the base platform being used, it does not affect modifier application (they get skipped all the same).

Setting the modifier on the CLI

This "works", but is not compatible with my actual use case, where we have modifiers set deep in the build graph, and asking users to also know about them and set them on the CLI is not reasonable.

Using target_compatible_with

Using target_compatible_with instead of (or besides) modifiers is useful to catch missing constraints earlier in the build, but it does not solve the issue: end-users still need to specify all constraints at the top-level target being built, no matter how deep in the build tree those constraints are being used, and how irrelevant they are to the end user.

This is a considerable abstraction leak.

Transistions

Transitions can be used to add constraints to some parts of the build tree.

We could use them for this purpose, but they are significantly more cumbersome to use, less user friendly, and our understanding is that modifiers are supposed to replace them for this use case.

[cormacrelf]

The original configuration modifiers RFC is no longer accessible to confirm or deny that this is intended behaviour, but I do think it is. Modifiers apply to the whole graph (until a transition), and they apply based on matching the target that you are directly building or running. That's how they work in PACKAGE / set_cfg_modifiers. It is only by virtue of building a target contained in a PACKAGE tree that you get the modifiers, but when you do, those modifiers affect most of the configuration for all the dependencies because attrs.dep() requests a dep of the same configuration. Same goes for modifiers directly attached to a target. They apply when built directly.

I don't really know what it would mean if modifiers applied when something was used as a dependency. Like, two libraries B and C are dependencies of a binary A. B and C both specify a modifier, one says debug and the other says release. I guess you want A to depend on B(debug) and C(release)? What do I do about diamond dependency D? Do I build it twice? Do I error? Should A be built in debug or release? I don't think there's a good answer to any of those questions.

Certainly you could reframe this as a feature request rather than a bug, but have you tried target_compatible_with? It seems to do what you want, enforcing that a target is built with a certain modifier set or it is built via a transition.

[cbarrete]

I guess you want A to depend on B(debug) and C(release)?

Yes, this is actually a common use case (e.g. in video games), to want a debug build where some dependencies are built in release mode because they are critical to the debug build being usable at all.

What do I do about diamond dependency D? Do I build it twice? Do I error? Should A be built in debug or release? I don't think there's a good answer to any of those questions.

I don't have a good answer either, but I think that erroring out is fine, at least at first, because it does not commit us to anything.

have you tried target_compatible_with? It seems to do what you want, enforcing that a target is built with a certain modifier set or it is built via a transition.

I have! I forgot to add this to the list of things I've considered/tried, and will edit the issue to reflect that. It is better than nothing because it explains to the user why their build fails, but it still imposes them the burden of adding those modifiers everywhere they depend on a target, which is a massive abstraction leak, and simply doesn't scale.

Certainly you could reframe this as a feature request rather than a bug

I could, but it was reported to me as a bug, because a well formed target that builds successfully doesn't build when depended on. It would be fine if there was another mechanism to solve that issue, but there isn't, short of maybe introducing a custom transition, but this is heavyweight, and is supposed to be mostly replaced by modifiers.

And if the intent is for modifiers to work like you are describing, then I think that target-level modifiers should come with a big warning about that footgun, which is that they are basically ignored if the target is not being built directly, but being depended on instead.

[cormacrelf]

Yes, this is actually a common use case (e.g. in video games), to want a debug build where some dependencies are built in release mode because they are critical to the debug build being usable at all.

We do that by making the debug build just add optimisation flags for those dependencies, directly on the target definition, or via a macro used for a third party dependency graph etc. This means you don't need to double up any dependencies in a single build mode, no problem with diamond deps, etc. The only downside would be if your release mode flags were really complicated or arch-specific and had to be replicated. But generally you want debug symbols AND optimisations. So it has to be different from release mode. Also, doing it this way means you only optimise the deps you say, not necessarily all of their transitive deps.

[target_compatible_with] is better than nothing because it explains to the user why their build fails, but it still imposes them the burden of adding those modifiers everywhere they depend on a target, which is a massive abstraction leak, and simply doesn't scale.

Yes, you do want to make it hard to hold wrong. CLI modifiers are for things you sometimes need, like asan etc. But for stuff that isn't optional, like CPU arch-specific targets, you should be structuring your build graph to do that automatically.

One way to achieve that is with configuration transitions. The new incoming_transition API looks very good, it lets you define a transition like "add this mandatory config" as a named target itself, and make every dep edge to a particular target go through that transition. For example many targets might be wasm-only, and you can make the publicly accessible targets be wasm bundles that force a transition to wasm always.

[cbarrete]

Yeah, that particular example was maybe not the best example, but my point is, we have lots of build-time configuration that we want to set on various parts of the build graph.

One way to achieve that is with configuration transitions.

We've used transitions in the past, I'm quite familiar with them. But they are very cumbersome, especially for fine grained configuration. It is/would be much nicer to just be able to slap a modifiers = [] on a target instead.

Also, transitions have the downside that they are unconditional as far as I know. In my example, if the user wants to override the build mode, they can do so via the CLI. With transitions, the target would always be run in release mode AFAIU.

But for stuff that isn't optional, like CPU arch-specific targets, you should be structuring your build graph to do that automatically.

Absolutely, and that's how I'm trying to use the modifiers attribute.

If it cannot work this way, then I don't see the point of having it on most rules at all then, especially library ones.

We'll probably use transitions until someone from Meta gives us some insights as to how this should be approached, but it's suboptimal from my point of view.

[8Keep]

It is intentional, but the modifiers rfc doc is not public anymore. We have a lot of RFC's that are now only visible internally. Tagging @scottcao, is there any way to make these public again? I assume they were made internal for a reason.

The short version is, for most cases you use a transition, modifiers should be used to avoid diamond dependency problems and toolchain issues. Sanitizers, toolchain versioning, third party versioning, and almost all other options, you usually want something to apply to the whole dependency graph, and not have a transition in the middle of the graph.

What people use internally for one small transition within the graph are constraint_overrides. That's supported on most rules already. It's similar to modifiers, except it applies for all transitive deps downward, and it'll work even if it in the middle of the graph.

There may be some migration away from constraint_overrides toward something with a better api in the future, so it may not be publicized much externally.

[cbarrete]

Hey @8Keep, thanks for the reply and sorry about the delay - the notification for this got lost somehow.

What people use internally for one small transition within the graph are constraint_overrides. That's supported on most rules already. It's similar to modifiers, except it applies for all transitive deps downward, and it'll work even if it in the middle of the graph.

Thanks a lot for pointing this out, I'll take a look.

There may be some migration away from constraint_overrides toward something with a better api in the future, so it may not be publicized much externally.

Noted, thanks!

I will try to find some time to document some of information in this thread after #1191 gets merged (I still need to address some comments on it right now).