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

Broken repository name handling for build_settings and label_flag #10499

Closed
moroten opened this issue Dec 30, 2019 · 5 comments
Closed

Broken repository name handling for build_settings and label_flag #10499

moroten opened this issue Dec 30, 2019 · 5 comments
Assignees
Labels
P3 We're not considering working on this, but happy to review a PR. (No assignee) Starlark configuration Starlark transitions, build_settings team-Configurability platforms, toolchains, cquery, select(), config transitions

Comments

@moroten
Copy link
Contributor

moroten commented Dec 30, 2019

Description of the problem:

When using a label_flag that is defined in the main repository, it must be referred to as //:my_label_flag on the command line and in transitions. Using @my_repo//:my_label_flag will not work but will also not render any error message.

If the same repository is used as an external, using by @my_repo//:my_label_flag is a must in the transitions as //:my_label_flag always points to the main repository.

Basically, the functionality is there, but the repository name handling is broken.

Expected behaviour

Transitions should not allow build_setting names without @, either @//... or @my_repo//... should be used. Using //... should render an error suggesting @my_repo//... or @//..., apart from when using the built in //command_line_option:....

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

cat <<EOF > WORKSPACE
local_repository(
    name = "sub",
    path = "sub",
)
EOF

mkdir sub
cat <<EOF > sub/WORKSPACE
workspace(name = "sub")
EOF

cat <<EOF > sub/BUILD.bazel
load(":rules.bzl", "rule_with_transition")
package(default_visibility = ["//visibility:public"])
label_flag(
    name = "the_label_flag",
    build_setting_default = ":bad_filegroup",
)
filegroup(name = "bad_filegroup", srcs = [])
filegroup(name = "good_filegroup", srcs = [])

rule_with_transition(
    name = "output_target",
    srcs = [":the_label_flag"],
)
EOF

cat <<EOF > sub/rules.bzl
def _my_transition_impl(settings, attr):
    print(settings)
    return {
        '@sub//:the_label_flag': Label("@sub//:good_filegroup"),
    }

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

def _rule_impl(ctx):
    print(ctx.attr.srcs)

rule_with_transition = rule(
    implementation = _rule_impl,
    # cfg = my_transition,
    attrs = {
        "srcs": attr.label_list(allow_files = True, cfg = my_transition),
        "_whitelist_function_transition": attr.label(default = "@bazel_tools//tools/whitelists/function_transition_whitelist"),
    },
)
EOF

Now run

$ bazel build @sub//:output_target
DEBUG: .../external/sub/rules.bzl:2:5: {"@sub//:the_label_flag": Label("@sub//:bad_filegroup")}
DEBUG: .../external/sub/rules.bzl:14:5: [<alias target @sub//:the_label_flag of @sub//:good_filegroup>]  <---- CORRECT

$ bazel build @sub//:output_target --@sub//:the_label_flag=@sub//:good_filegroup
DEBUG: .../external/sub/rules.bzl:2:5: {"@sub//:the_label_flag": Label("@sub//:good_filegroup")}  <--- CORRECT
DEBUG: .../external/sub/rules.bzl:14:5: [<alias target @sub//:the_label_flag of @sub//:good_filegroup>]  <--- Still CORRECT

$ (cd sub && bazel build output_target)
DEBUG: ./sub/rules.bzl:2:5: {"@sub//:the_label_flag": Label("@sub//:bad_filegroup")}
DEBUG: ./sub/rules.bzl:14:5: [<alias target //:the_label_flag of //:bad_filegroup>]  <--- WRONG

$ (cd sub && bazel build output_target --//:the_label_flag=:good_filegroup)
DEBUG: ./sub/rules.bzl:2:5: {"@sub//:the_label_flag": Label("//:bad_filegroup")}  <--- WRONG
DEBUG: ./sub/rules.bzl:14:5: [<alias target //:the_label_flag of //:good_filegroup>]  <--- CORRECT

$ (cd sub && bazel build output_target --@sub//:the_label_flag=:good_filegroup)
DEBUG: ./sub/rules.bzl:2:5: {"@sub//:the_label_flag": Label("//:good_filegroup")}  <--- CORRECT
DEBUG: ./sub/rules.bzl:14:5: [<alias target //:the_label_flag of //:bad_filegroup>]  <--- WRONG

Remove @sub from sub/rules.bzl and you will get

$ bazel build @sub//:output_target
ERROR: Analysis of target '@sub//:output_target' failed; build aborted: no such package '': BUILD file not found in any of the following directories. Add a BUILD file to a directory to mark it as a package.
^--- This error is because ./BUILD.bazel does not exist to define @//:the_label_flag.

$ bazel build @sub//:output_target --@sub//:the_label_flag=@sub//:good_filegroup
ERROR: Analysis of target '@sub//:output_target' failed; build aborted: no such package '': BUILD file not found in any of the following directories. Add a BUILD file to a directory to m
ark it as a package.
^--- This error is because ./BUILD.bazel does not exist to define @//:the_label_flag.

$ (cd sub && bazel build output_target)
DEBUG: ./sub/rules.bzl:2:5: {"//:the_label_flag": Label("//:bad_filegroup")}
DEBUG: ./sub/rules.bzl:14:5: [<alias target //:the_label_flag of //:good_filegroup>]  <--- CORRECT

$ (cd sub && bazel build output_target --//:the_label_flag=:good_filegroup)
DEBUG: ./sub/rules.bzl:2:5: {"//:the_label_flag": Label("//:good_filegroup")}  <--- CORRECT
DEBUG: ./sub/rules.bzl:14:5: [<alias target //:the_label_flag of //:good_filegroup>]  <--- CORRECT

$ (cd sub && bazel build output_target --@sub//:the_label_flag=:good_filegroup)
DEBUG: ./sub/rules.bzl:2:5: {"//:the_label_flag": Label("//:bad_filegroup")}  <--- WRONG
DEBUG: ./sub/rules.bzl:14:5: [<alias target //:the_label_flag of //:good_filegroup>]  <--- CORRECT

What operating system are you running Bazel on?

Linux

What's the output of bazel info release?

release 2.0.0

Have you found anything relevant by searching the web?

Nothing found when searching for label_flag on GitHub issues: https://github.com/bazelbuild/bazel/issues?utf8=%E2%9C%93&q=is%3Aissue+label_flag

Relates to bazel-dicsuss platforms, build_settings, transitions and features.

The example above is loosely based on https://github.com/bazelbuild/bazel/blob/master/src/test/shell/integration/starlark_configurations_test.sh.

@gregestren gregestren self-assigned this Jan 10, 2020
@gregestren gregestren added the P2 We'll consider working on this in future. (Assignee optional) label Jan 10, 2020
@gregestren
Copy link
Contributor

I'm a bit weak on repositories but I can at least triage.

@moroten
Copy link
Contributor Author

moroten commented Feb 8, 2020

I can confirm that the problem is not just for label_flag, it also applies to string_flag. With the code below, use the same command lines but with e.g. --@sub//:the_setting=good instead.

cat <<EOF > WORKSPACE
local_repository(
    name = "sub",
    path = "sub",
)
EOF

mkdir sub
cat <<EOF > sub/WORKSPACE
workspace(name = "sub")
EOF

cat <<EOF > sub/BUILD.bazel
load(":rules.bzl", "rule_with_transition", "string_flag")
package(default_visibility = ["//visibility:public"])
string_flag(
    name = "the_setting",
    build_setting_default = "bad",
)

rule_with_transition(
    name = "output_target",
    string_value = ":the_setting",
)
EOF

cat <<EOF > sub/rules.bzl
BuildSettingInfo = provider(
    fields = {"value": "The value."}
)

def _impl(ctx):
    return BuildSettingInfo(value = ctx.build_setting_value)

string_flag = rule(
    implementation = _impl,
    build_setting = config.string(flag = True),
)

def _my_transition_impl(settings, attr):
    print(settings)
    return {
        "@sub//:the_setting": "good",
    }

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

def _rule_impl(ctx):
    print(ctx.attr.string_value[BuildSettingInfo])

rule_with_transition = rule(
    implementation = _rule_impl,
    cfg = my_transition,
    attrs = {
        "string_value": attr.label(),
        "_whitelist_function_transition": attr.label(default = "@bazel_tools//tools/whitelists/function_transition_whitelist"),
    },
)
EOF

@gregestren
Copy link
Contributor

I'm having trouble prioritizing this at the moment. I'm going to refer to @juliexxia for better triage, and check if she has context that I don't.

@juliexxia I don't understand the intersection of SBC and repositories very well. Feel free to own this or assign to someone else, but also feel free to assign back to me if there's no better obvious owner.

I also appreciate input from anyone on the right impact priority.

@gregestren gregestren added team-Configurability platforms, toolchains, cquery, select(), config transitions Starlark configuration Starlark transitions, build_settings and removed team-Front-End labels Feb 13, 2020
@moroten moroten changed the title Broken repository name handling for label_flag Broken repository name handling for build_settings and label_flag Mar 19, 2020
@gregestren gregestren added P3 We're not considering working on this, but happy to review a PR. (No assignee) and removed P2 We'll consider working on this in future. (Assignee optional) labels May 11, 2020
@gregestren
Copy link
Contributor

Let's consolidate these bugs into #11128.

When that one is fixed, it if doesn't address this we can re-open this.

bazel-io pushed a commit that referenced this issue Jan 26, 2021
This brings us home to the principle of "we always deal with canonicalized settings on the back end". Before this CL, this situation described in #10499 was still an issue.

Some background:

By design, the label-object map of starlark options in BuildOptions does not store starlark flags that are set to their default value (set either by never having been set, or explicitly set to the default value). This means before we do starlark transitions, we fill out the configuration with all the relevant default values so they're there if we need to read them. Before this CL, those editions to the map were being added in their given form, not there canonical form. This obviously caused issues when our transition logic tried to find these setting using their canonical form. This CL fixes that. The affected logic is ConfigurationResolver#addDefaultStarlarkOptions.

Fixes #11128

PiperOrigin-RevId: 353923997
@juliexxia
Copy link
Contributor

Wanted to express thanks for the detail of this issue report. Helped immensely with writing appropriate tests when writing this fix. Should be all fixed now, thanks for your patience.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P3 We're not considering working on this, but happy to review a PR. (No assignee) Starlark configuration Starlark transitions, build_settings team-Configurability platforms, toolchains, cquery, select(), config transitions
Projects
None yet
Development

No branches or pull requests

4 participants