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

bool_flag in config_setting via alias seems broken #13463

Closed
barbibulle opened this issue May 11, 2021 · 23 comments
Closed

bool_flag in config_setting via alias seems broken #13463

barbibulle opened this issue May 11, 2021 · 23 comments
Assignees
Labels
not stale Issues or PRs that are inactive but not considered stale P3 We're not considering working on this, but happy to review a PR. (No assignee) team-Configurability platforms, toolchains, cquery, select(), config transitions type: bug
Milestone

Comments

@barbibulle
Copy link

Description of the problem / feature request:

When referencing a bool_flag in a config_setting via an alias indirection, the flag value set on the command line seems to be ignored.
NOTE: it isn't necessarily only broken for bool_flags, but that's just a simple example that illustrate the issue. I suspect other flag types would have the same problem.

Feature requests: what underlying problem are you trying to solve with this feature?

I want to enable some sort of late-binding for enabling/disabling features, in a way that would 'point' to different workspaces based on a flag (boolean or otherwise). Since select() isn't possible in a WORKSPACE, it seems that a recommended alternative is to go through a select, config_setting + alias indirection.
I have tried to simplify my example to a minimum size, so this doesn't exactly illustrate what I'm trying to archive, but it does illustrate the bug I see when following that approach

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

With the following files, run run_all.sh which will run 4 builds, one by one, and execute the result, printing feature a: 0 or feature a: 1 depending on the outcome of the select in the BUILD file.
As we can see, we get 0 and 1 (depending on the --//:enable_feature_a=xxx command line option) when the select is for a config_setting that references a bool_flag directly, but we always get 0 regardless of the command line option when using a config_setting that references a bool_flag via an alias.

❯ cat 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",
)
load("@bazel_skylib//:workspace.bzl", "bazel_skylib_workspace")
bazel_skylib_workspace()

❯ cat BUILD 
load("@bazel_skylib//rules:common_settings.bzl", "bool_flag")

config_setting(
    name = "feature_a_enabled",
    flag_values = {
        ":enable_feature_a": "true"
    }
)

config_setting(
    name = "feature_a_enabled_aliased",
    flag_values = {
        ":enable_feature_a_aliased": "true"
    }
)

bool_flag(
    name = "enable_feature_a",
    build_setting_default = False,
)

alias(
    name = "enable_feature_a_aliased",
    actual = "enable_feature_a"
)

cc_binary(
    name = "my_binary",
    srcs = ["main.c"] + select({
        ":feature_a_enabled": ["feature_a.c"],
        "//conditions:default": ["default_a.c"]
    })
)

cc_binary(
    name = "my_binary_aliased",
    srcs = ["main.c"] + select({
        ":feature_a_enabled_aliased": ["feature_a.c"],
        "//conditions:default": ["default_a.c"]
    })
)

❯ cat main.c 
#include <stdio.h>

extern int get_feature_a_value(void);

int main(int argc, char** argv) {
    printf("feature a: %d\n", get_feature_a_value());
}

❯ cat feature_a.c 
int get_feature_a_value(void) {
    return 1;
}

❯ cat default_a.c 
int get_feature_a_value(void) {
    return 0;
}

❯ cat run_all.sh 
bazel build --//:enable_feature_a=false :my_binary
bazel-bin/my_binary
bazel build --//:enable_feature_a=true :my_binary
bazel-bin/my_binary

bazel build --//:enable_feature_a=false :my_binary_aliased
bazel-bin/my_binary_aliased
bazel build --//:enable_feature_a=true :my_binary_aliased
bazel-bin/my_binary_aliased

What operating system are you running Bazel on?

macOS Big Sur 11.3.1

What's the output of bazel info release?

release 4.0.0

Have you found anything relevant by searching the web?

Looked online, couldn't find anything.
The original reference to using alias with select when selecting things in a WORKSPACE was found here:
https://docs.bazel.build/versions/master/configurable-attributes.html
(see "Why doesn’t select() work with bind()?")

@aiuto aiuto added team-Configurability platforms, toolchains, cquery, select(), config transitions untriaged labels May 13, 2021
@gregestren
Copy link
Contributor

Interesting observation thanks. I'll dig in some to find the root cause and report back...

@gregestren
Copy link
Contributor

Some breakpoint debugging shows me this is what fails:

Object configurationValue =
optionDetails.getOptionValue(specifiedLabel) != null
? optionDetails.getOptionValue(specifiedLabel)
: provider.getDefaultValue();

optionDetails has the registered value for :enable_feature_a (the original flag). But it's being called with specifiedLabel=: enable_feature_a_aliased. So optionDetails.getOptionValue returns null, which means it returns the flag's default value (False).

fd2c682 has some precedent. I think extending some of its setup logic to user-defined flags can fix this easily enough.

@gregestren
Copy link
Contributor

I prototyped the change, basically change to:

Object configurationValue =
    optionDetails.getOptionValue(actualLabel) != null
          ? optionDetails.getOptionValue(actualLabel)
          : provider.getDefaultValue(); 

It works, with one blocking caveat. There's already code to support label_flag in the opposite way:
83f8d18

label_flag is implemented internally as an alias. And that code expects to be able to select() against its name vs. what it points to. Inside ConfigSetting there's no way to distinguish this from any other alias.

So we'd need either some way to distinguish them, and implement different semantics accordingly, or change the semantics so they're both interpreted the same way.

@gregestren gregestren added P3 We're not considering working on this, but happy to review a PR. (No assignee) type: bug and removed untriaged labels Jun 7, 2021
@gregestren
Copy link
Contributor

Re: label_flag implementation, see

public static class LabelBuildSettingRule extends CommonAliasRule<BuildConfiguration> {
.

@nikhilpothuru
Copy link

Hi @gregestren , I'm new here and I'm looking through this ticket. Would it be fine if I pick this ticket up?

@gregestren
Copy link
Contributor

Hi @nikhilpothuru - you're welcome to. Do you feel like you can navigate effectively the conceptual challenge above of

So we'd need either some way to distinguish them, and implement different semantics accordingly, or change the semantics so they're both interpreted the same way.

?

The smallest user footprint way would probably be to tell whether an alias is a label_flag vs. a bool_flag, then implement each one's logic accordingly.

@nikhilpothuru
Copy link

Hi @gregestren, I take a look at this over the week and let you know if this is something I can handle. As I look through this ticket, I'll add any questions I have on this thread.

@nikhilpothuru
Copy link

Hi @gregestren, Is there any documentation for setting up the development environment? (i.e. setting up the debugger)

@gregestren
Copy link
Contributor

@aiuto aiuto added this to the flags cleanup milestone Dec 10, 2021
@github-actions
Copy link

github-actions bot commented Jun 4, 2023

Thank you for contributing to the Bazel repository! This issue has been marked as stale since it has not had any activity in the last 1+ years. It will be closed in the next 14 days unless any other activity occurs or one of the following labels is added: "not stale", "awaiting-bazeler". Please reach out to the triage team (@bazelbuild/triage) if you think this issue is still relevant or you are interested in getting the issue resolved.

@github-actions github-actions bot added the stale Issues or PRs that are stale (no activity for 30 days) label Jun 4, 2023
@fmeum
Copy link
Collaborator

fmeum commented Jun 4, 2023

@bazelbuild/triage Not stale

@sgowroji sgowroji added not stale Issues or PRs that are inactive but not considered stale and removed stale Issues or PRs that are stale (no activity for 30 days) labels Jun 4, 2023
@jacky8hyf
Copy link

Could you please prioritize this? Kleaf hits this issue too, so we just put in a wonky workaround here:

https://cs.android.com/android/kernel/superproject/+/common-android-mainline:build/kernel/kleaf/common_kernels.bzl;l=929;drc=8e7727c052aef0b328cd6dfa94121f7ef7d9f77f

It is not urgent, but definitely annoying.

@gregestren
Copy link
Contributor

@lberki noted this as a possible task for people looking for tasks. Still happily accepting contributors. This bug has some nice guidance in its history.

@afq984
Copy link

afq984 commented Nov 21, 2023

We also encountered this on ChromeOS.

We are generating alias so developers don't have to type the full package paths. For example we have this alias set up:

% BOARD=amd64-generic bazel query @portage//target/sys-apps/attr:2.5.1 --output=build
(22:43:22) INFO: Current date is 2023-11-21
# /usr/local/google/home/aaronyu/.cache/bazel/_bazel_aaronyu/a5468eaf077d9e25965baa9c750fa428/external/_main~portage~portage/target/sys-apps/attr/BUILD.bazel:7:6
alias(
  name = "2.5.1",
  visibility = ["//bazel:internal"],
  actual = select({"//bazel/portage:stage1": "@_main~portage~portage//internal/packages/stage1/target/board/portage-stable/sys-apps/attr:2.5.1", "//bazel/portage:stage2": "@_main~portage~portage//internal/packages/stage2/target/board/portage-stable/sys-apps/attr:2.5.1"}),
)

We hit this problem when we want to generate some target-specific build flags, along with the alias where @portage//target/sys-apps/attr:incremental points to the bool_flag "@_main~portage~portage//internal/packages/stage2/target/board/portage-stable/sys-apps/attr:incremental"

--@portage//target/sys-apps/attr:incremental doesn't work in the command line.

@jin jin self-assigned this Nov 24, 2023
@lberki lberki self-assigned this Nov 24, 2023
@lberki
Copy link
Contributor

lberki commented Nov 24, 2023

I have an internal change out to fix this. Hilariously, @jin just started working on this 2.5 years old bug the exact moment I did :(

@afq984
Copy link

afq984 commented Dec 17, 2023

Any chance we get this in Bazel 7?

@fmeum
Copy link
Collaborator

fmeum commented Dec 17, 2023

@bazel-io flag

@bazel-io bazel-io added the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label Dec 17, 2023
@lberki
Copy link
Contributor

lberki commented Dec 18, 2023

@afq984 Bazel 7 has already been released, would Bazel 7.1 work?

@afq984
Copy link

afq984 commented Dec 18, 2023

Apologies, I confused this issue with #20582. We were aliasing on the flag, not config_setting.
I think we don't need this one.

@iancha1992
Copy link
Member

@bazel-io fork 7.1.0

@bazel-io bazel-io removed the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label Dec 18, 2023
@brentleyjones
Copy link
Contributor

@iancha1992 Candidate for 7.0.1?

@lberki
Copy link
Contributor

lberki commented Dec 19, 2023

@brentleyjones I don't think so, unless @meteorcloudy pushes hard for it. 7.0.1 is to fix serious regressions that slipped through into 7.0.0 and this doesn't look like one.

bazel-io pushed a commit to bazel-io/bazel that referenced this issue Dec 21, 2023
Previously, if a config_setting referenced a Starlark setting through an alias, it was always evaluated as if the setting had its default value. Now, it is evaluated correctly.

This is done by looking up the value of the build setting in the configuration based on its own label as specified in BuildSettingProvider.

Fixes bazelbuild#13463 .

RELNOTES: None.
PiperOrigin-RevId: 585658985
Change-Id: Id534cd95282355f1143302bf703145bb53708a41
github-merge-queue bot pushed a commit that referenced this issue Jan 4, 2024
Previously, if a config_setting referenced a Starlark setting through an
alias, it was always evaluated as if the setting had its default value.
Now, it is evaluated correctly.

This is done by looking up the value of the build setting in the
configuration based on its own label as specified in
BuildSettingProvider.

Fixes #13463 .

RELNOTES: None.
Commit
d44c3f9

PiperOrigin-RevId: 585658985
Change-Id: Id534cd95282355f1143302bf703145bb53708a41

Co-authored-by: Googler <lberki@google.com>
Co-authored-by: Ian (Hee) Cha <heec@google.com>
@iancha1992
Copy link
Member

A fix for this issue has been included in Bazel 7.1.0 RC1. Please test out the release candidate and report any issues as soon as possible. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
not stale Issues or PRs that are inactive but not considered stale P3 We're not considering working on this, but happy to review a PR. (No assignee) team-Configurability platforms, toolchains, cquery, select(), config transitions type: bug
Projects
None yet
Development

No branches or pull requests