bazelbuild / bazel

a fast, scalable, multi-language and extensible build system
https://bazel.build
Apache License 2.0
23.02k stars 4.03k forks source link

`@bazel_tool//tools/cpp:link_extra_libs` should not propagate to exec config #22457

Open tpudlik opened 4 months ago

tpudlik commented 4 months ago

Description of the feature request:

The C++ compilation rules have a neat feature: they allow you to specify a library that should be a linktime dependency of all cc_binary targets. That library is configured through a label flag, @bazel_tool//tools/cpp:link_extra_libs.

Unfortunately, this label flag persists across the transition from the target to the exec configuration. This is very inconvenient: if the link_extra_libs you want for the target platform include generated code, you run into circular dependencies when trying to build any binaries used in their generation. This tends to happen with some Pigweed libraries, preventing us from using link_extra_libs in most cases.

@gregestren I recall that internally we've done some work to restrict flag propagation to the exec config (b/292617118). What's the status on the open source side?

Which category does this issue belong to?

Configurability

What underlying problem are you trying to solve with this feature?

No response

Which operating system are you running Bazel on?

No response

What is the output of bazel info release?

release 7.1.2

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

No response

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

No response

Have you found anything relevant by searching the web?

No response

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

No response

gregestren commented 4 months ago

I'm sure this is just a matter of flipping https://github.com/bazelbuild/bazel/blob/4d2250561fc7b16b35eb99cb51921ec530457a4a/src/main/java/com/google/devtools/build/lib/analysis/config/CoreOptions.java#L125-L126.

We flipped that in the org-wide .bazelrc in b/292617118 but not by default.

I think it's reasonable to flip it here, combined with an announcement to bazel@head users on how to fix potential breakages.

How about I put up a PR and we'll see if standard Bazel CI passes as a starting point?

fmeum commented 4 months ago

@gregestren I'm in favor of flipping it, but we should follow the incompatible change procedure.

Cc @meteorcloudy

tjgq commented 4 months ago

Is it a good idea to default --experimental_exclude_starlark_flags_from_exec_config to true? I think this is going to be extremely non-intuitive. What if the ability to propagate to exec was a property of the configuration flag, instead of determined by a separate command line option, i.e. label_flag(..., propagate_to_exec = True)? That way, it's harder to miss that you need to make an explicit decision when defining the flag.

tpudlik commented 4 months ago

Ah, I didn't realize that --experimental_exclude_starlark_flags_from_exec_config is just a flag that I can add to my .bazelrc to enable (pre-adopt) this behavior. I don't think it's documented anywhere! Let me enable this as a workaround.

As a user, I personally would like to see this flag flipped, but I haven't looked deeply into the pros and cons. What I do feel strongly about (as a developer of a library that's built in both places) is that we should set the flag to the same default value both internally and in open-source Bazel!

fmeum commented 4 months ago

Is it a good idea to default --experimental_exclude_starlark_flags_from_exec_config to true? I think this is going to be extremely non-intuitive. What if the ability to propagate to exec was a property of the configuration flag, instead of determined by a separate command line option, i.e. label_flag(..., propagate_to_exec = True)? That way, it's harder to miss that you need to make an explicit decision when defining the flag.

@gregestren What do you think about:

  1. adding a propagate_to_exec flag to build settings that defaults to True.
  2. turning --experimental_exclude_starlark_flags_from_exec_config into an incompatible flag that flips the default to False.
  3. flipping the incompatible flag.
gregestren commented 4 months ago

@aranguyen has a proposal for a proper flag propagation model that they intend to implement in June. Which I think would cover 1. I'll see if I can share the proposal next week.

That sequence generally sounds fine.

gregestren commented 2 months ago

Update:

I'm now confident we should follow the process described in the last two comments.

As this thread notes, the complication is if someone needs their Starlark flag to still propagate, you can only do that in a .bazelrc. Which doesn't generalize well.

What I do feel strongly about (as a developer of a library that's built in both places) is that we should set the flag to the same default value both internally and in open-source Bazel!

Agreed. The current discrepancy is because Google has its global .bazelc that solves this problem. Bazel of course doesn't.

Here's the sequence:

  1. Let @aranguyen 's customizable flag propagation support finish. This makes it possible for Bazel to understand and apply flag annotations like "only propagate to X".
  2. Add a "propagate_to_exec" flag to build settings, as @fmeum notes
  3. Rename / flip --experimental_exclude_starlark_flags_from_exec_config

1 is well underway. We'll follow up when the implementation lands.

armandomontanez commented 2 weeks ago

How far out is propagate_to_exec? I hit a use case where 100% of the uses of one of my flags requires the propagate_to_exec attribute. There's a feature flag for enabling the new rules_cc toolchain API, and it lives behind the magical transitions Bazel does when handling toolchains: https://github.com/bazelbuild/rules_cc/blob/5c1be25800e0806356624c1effd7a23240b3a45e/cc/toolchains/impl/toolchain_config.bzl#L106

With --experimental_exclude_starlark_flags_from_exec_config, the flag cannot be controlled via the command-line.

gregestren commented 1 week ago

How far out is propagate_to_exec?

It'd be great to aim for October @ bazel head.

the flag cannot be controlled via the command-line.

Can you clarify this point?

armandomontanez commented 1 week ago

For reasons I haven't dug too deeply into, with --experimental_exclude_starlark_flags_from_exec_config enabled I am unable to flip the --@rules_cc//cc/toolchains:experimental_enable_rule_based_toolchains flag either from a .bazelrc or via specifying it as a bazel build CLI flag. I believe the root of this issue stems from the fact that the flag evaluation happens as a dependency of a toolchain rule, which does some number of transitions.

I tried allowlisting this flag by doing this in my .bazelrc:

build --experimental_propagate_custom_flag=@rules_cc//cc/toolchains:experimental_enable_rule_based_toolchains
build --@rules_cc//cc/toolchains:experimental_enable_rule_based_toolchains=True

But it still didn't take effect.

gregestren commented 1 week ago

@armandomontanez do you have a simple reproduction?

That doesn't sound like something propagate_to_exec would change, since that's just moving the source of truth vs. changing the underlying semantics.