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

Support workspace label prefixes in Starlark config and platform APIs #11128

Closed
gregestren opened this issue Apr 14, 2020 · 16 comments
Closed

Support workspace label prefixes in Starlark config and platform APIs #11128

gregestren opened this issue Apr 14, 2020 · 16 comments
Labels
P2 We'll consider working on this in future. (Assignee optional) team-Configurability platforms, toolchains, cquery, select(), config transitions type: feature request

Comments

@gregestren
Copy link
Contributor

gregestren commented Apr 14, 2020

This is a catch-all to focus the common theme of:

At the narrowest this may require unrelated fixes to build_setting and toolchain resolution logic. But the general theme of the problem is clear, however the fixes are made.

We're (Bazel configurability) trying to prioritize this for the current quarter.

@gregestren gregestren added P2 We'll consider working on this in future. (Assignee optional) team-Configurability platforms, toolchains, cquery, select(), config transitions labels Apr 14, 2020
@gregestren
Copy link
Contributor Author

Also #10118.

@aherrmann
Copy link
Contributor

#11734 is a similar issue in aspects.

@gregestren
Copy link
Contributor Author

@juliexxia - can you advise on how much of this issue is still relevant?

@nacl
Copy link

nacl commented Nov 16, 2020

With 3.7, our experience with this is mostly the same as what's described in #9177, except Bazel no longer crashes, instead, the flag doesn't seem to take effect when it's prefixed with the workspace id.

Interestingly, when I run these two commands (within the example in #9177), it seems like the prefixed and non-prefixed versions are being treated as separate, but still work :

> bazel build //:my_drink --//:favorite_flavor=True
DEBUG: .../tmp/bazel-user-settings-example/rules.bzl:5:14: Get the opposite of default (True)
...
INFO: Build completed successfully, 1 total action
> bazel build //:my_drink --@bsws//:favorite_flavor=True
INFO: Build options --//:favorite_flavor and --@bsws//:favorite_flavor have changed, discarding analysis cache.
DEBUG: .../tmp/bazel-user-settings-example/rules.bzl:7:14: Get the default (False)
...
INFO: Build completed successfully, 1 total action

Referencing build settings from external WORKSPACEs (#9177 (comment)) seems to work well.

@stefanbucur
Copy link

I am experiencing the same behavior when it comes to using the flags in Bazel transitions.

For a WORKSPACE file defined as workspace(name = "my_workspace") and a build setting in my BUILD file as:

load("@bazel_skylib//rules:common_settings.bzl", "string_flag")

string_flag(
    name = "my_flag",
    build_setting_default = "none",
    visibility = ["//visibility:public"],
)

Then setting --@my_workspace//:my_flag and --//:my_flag end up changing distinct values. A Bazel transition on //:my_flag will not be affected when setting @my_workspace//:my_flag. I can actually list both versions as inputs to the transition, and they will be read as independent values.

@juliexxia
Copy link
Contributor

Then setting --@my_workspace//:my_flag and --//:my_flag end up changing distinct values. A Bazel transition on //:my_flag will not be affected when setting @my_workspace//:my_flag. I can actually list both versions as inputs to the transition, and they will be read as independent values.

@stefanbucur would you mind writing out a full command line/transition for the scenarios you describe here? From my own testing I believe settings --@my_workspace//:my_flag and //:my_flag from inside that workspace successfully resolve to a single flag.

@moroten
Copy link
Contributor

moroten commented Dec 1, 2020

@juliexxia @stefanbucur I've modified #10499 into the following:

cat <<EOF > WORKSPACE
workspace(name = "my_workspace")

load("@bazel_tools//tools/build_defs/repo:http.bzl", "http_archive")
http_archive(
    name = "bazel_skylib",
    urls = [
        "https://github.com/bazelbuild/bazel-skylib/releases/download/1.0.3/bazel-skylib-1.0.3.tar.gz",
        "https://mirror.bazel.build/github.com/bazelbuild/bazel-skylib/releases/download/1.0.3/bazel-skylib-1.0.3.tar.gz",
    ],
    sha256 = "1c531376ac7e5a180e0237938a2536de0c54d93f5c278634818e0efc952dd56c",
)
EOF

cat <<EOF > BUILD.bazel
load("@bazel_skylib//rules:common_settings.bzl", "string_flag")
load(":rules.bzl", "rule_with_transition")

string_flag(
    name = "my_flag",
    build_setting_default = "none",
    visibility = ["//visibility:public"],
)
package(default_visibility = ["//visibility:public"])

rule_with_transition(
    name = "my_target",
)
EOF

cat <<EOF > rules.bzl
def _my_transition_impl(settings, attr):
    print(settings)
    return settings

my_transition = transition(
    implementation = _my_transition_impl,
    inputs = ["//:my_flag", "@my_workspace//:my_flag"],
    outputs = ["//:my_flag", "@my_workspace//:my_flag"],
)

def _rule_impl(ctx):
    pass

rule_with_transition = rule(
    implementation = _rule_impl,
    cfg = my_transition,
    attrs = {
        "_allowlist_function_transition": attr.label(default = "@bazel_tools//tools/allowlists/function_transition_allowlist"),
    },
)
EOF

Running Bazel 3.7.0 gives:

$ ~/bin/bazel-3.7.0-linux-x86_64 build my_target --//:my_flag=foo --@my_workspace//:my_flag=bar
Starting local Bazel server and connecting to it...
DEBUG: /.../rules.bzl:2:10: {"//:my_flag": "foo", "@my_workspace//:my_flag": "bar"}
DEBUG: /.../rules.bzl:2:10: {"//:my_flag": "foo", "@my_workspace//:my_flag": "bar"}
INFO: Analyzed target //:my_target (6 packages loaded, 7 targets configured).
INFO: Found 1 target...
Target //:my_target up-to-date (nothing to build)
INFO: Elapsed time: 3.378s, Critical Path: 0.02s
INFO: 1 process: 1 internal.
INFO: Build completed successfully, 1 total action

@juliexxia
Copy link
Contributor

The general issue here seems to be that starlark options parsing and transitions aren't recognizing //:my_flag and @my_workspace//:my_flag as the same label.

This should be easy to fix during starlark options parsing - then in the command line you provided --@my_workspace//:my_flag=bar will simply overwrite --//:my_flag=foo and leave a configuration of //:my_flag=bar

Fixing this in transitions will take a little more digging. I'll keep looking.

@juliexxia
Copy link
Contributor

@moroten to confirm re: the transition issue. There's nothing that you want to do but can't right? Using the non-workspace-prefixed version of //:my_flag in the transition works fine, just using @my_workspace//:my_flag doesn't do what's expected, is that correct?

@juliexxia
Copy link
Contributor

@katre is taking a look at the issues described in #10927

@moroten
Copy link
Contributor

moroten commented Dec 3, 2020

@moroten to confirm re: the transition issue. There's nothing that you want to do but can't right? Using the non-workspace-prefixed version of //:my_flag in the transition works fine, just using @my_workspace//:my_flag doesn't do what's expected, is that correct?

We have worked around by avoiding user flags and settings in the main workspace. Instead, we have .bazelignored directories defined as local_repository where we put them instead.

@juliexxia
Copy link
Contributor

That sounds reasonable to me. Out of curiosity, why did y'all chose that instead of keeping flags in the main workspace and always referring to them in transitions/bazelrc without the repo name attached? //:my_flag should infer @//:my_flag aka the main workspace. Did y'all try that and it didn't work or did you just avoid it due to the confusion described in this issue?

@stefanbucur
Copy link

stefanbucur commented Dec 3, 2020

In our case, the reason is that the transition and the build configuration flags are part of a library that is meant to be used in other projects: https://github.com/bazelbuild/rules_fuzzing (more precisely, the code is still under review here: bazel-contrib/rules_fuzzing#86)

To simplify the specification of these flags on the command line, we encourage users to define their values as --configs in their own .bazelrc files, where they need to specify our workspace prefix (@rules_fuzzing) to refer to those, since our repository is imported as an http_archive. And to make things more uniform and easier to copy-paste, we also decided to use the workspace prefix in our own .bazelrc file.

Inside the transition implementation, it also wasn't clear to me how Bazel would interpret a //:flag label when the transition is imported in a different workspace. Will it assume it is part of the top-level repository, or inside the repository where the transition is defined? To eliminate any ambiguity, I ended up prepending the workspace prefix there too.

@stefanbucur
Copy link

Will it assume it is part of the top-level repository, or inside the repository where the transition is defined?

It turns out that the configuration flag does get interpreted in the context of the top-level workspace, so a rules repository library defining a transition on a configuration flag must reference that flag using the repository name. This means that our own .bazelrc file must reference those flags using the full repository name (see for example bazel-contrib/rules_fuzzing#91).

If this is intended behavior, does it mean that a rules library offering configuration flags must always be imported using the name matching the hard-coded repo name used in transitions?

@stefanbucur
Copy link

Somewhat related to this discussion, I discovered another weird behavior. For label-typed flags (the label_flag rule), using the repo name prefix (@my_workspace//:my_label_flag) on the command line has no effect on the configuration, regardless of whether the flag is used with or without the prefix in the Starlark code.

In our case, we use a label flag to control a cc_library dependency in a cc_test rule. Setting that flag with the repo name prefix has no effect - the dependency is left to its default value.

Not sure if this is a separate issue or a manifestation of the same underlying issue.

bazel-io pushed a commit that referenced this issue Jan 7, 2021
…and line as --@main_workspace//flag and --//flag will both parse to --//flag. Before this CL, the former maintained its workspace prefix and we would get different entries for these two formats.

work towards #11128

PiperOrigin-RevId: 350575360
bazel-io pushed a commit that referenced this issue Jan 7, 2021
…dConfigTransition so we can use it here and in FunctionTransitionUtil going forward

work towards #11128

PiperOrigin-RevId: 350588700
bazel-io pushed a commit that referenced this issue Jan 12, 2021
…ll stack in order to canonicalize.

There are 5 locations a user might write the label string of a build setting:
(1) on the command line
(2) in the transition definition inputs
(3) in the transition definition output
(4) in the transition impl function while reading the settings dict
(5) in the transition impl function return dict

We know that, depending on the situation, there are multiple ways to write "a build setting in the main repo of this project" - //myflag, @//myflag, @main_repo//my:flag.

In all 5 places listed above, bazel needs to recognize that the three different versions of //myflag above all map back to a single target. We need this for deduping purposes and consistency. 1 is taken care of in a grandparent CL to this CL. This Cl addresses locations 2-5.

To do this, whenever we are interpreting a user's input at one of the locations above, we covert that input string to the canonical string form of that build setting for our back end book keeping purposes. The logic to do this was modeled off of the `Label()` function logic. We also keep a map of canonicalized form to the user-given form in order to display messages (or key the dict in 4) to the user using the forms of the flags they used themselves.

work towards #11128

PiperOrigin-RevId: 351426279
@juliexxia
Copy link
Contributor

These should all be fixed as of this most recent commit.

bazel-io pushed a commit that referenced this issue May 7, 2021
This applies label canonicalization to starlark flags having a `--no`
prefix.

Relates to #11128.

Closes #13418.

PiperOrigin-RevId: 372619463
katre pushed a commit to katre/bazel that referenced this issue Jul 13, 2021
…and line as --@main_workspace//flag and --//flag will both parse to --//flag. Before this CL, the former maintained its workspace prefix and we would get different entries for these two formats.

work towards bazelbuild#11128

PiperOrigin-RevId: 350575360
katre pushed a commit to katre/bazel that referenced this issue Jul 13, 2021
…and line as --@main_workspace//flag and --//flag will both parse to --//flag. Before this CL, the former maintained its workspace prefix and we would get different entries for these two formats.

work towards bazelbuild#11128

PiperOrigin-RevId: 350575360
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-Configurability platforms, toolchains, cquery, select(), config transitions type: feature request
Projects
None yet
Development

No branches or pull requests

6 participants