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.

Sign up for GitHub

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

Jump to bottom

Convert Starlark options with Starlark#fromJava in build_options #18988

Closed
wants to merge 2 commits into from
Closed

Convert Starlark options with Starlark#fromJava in build_options #18988

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a31829a
Convert Starlark options with `Starlark#fromJava` in `build_config`
fmeum Jul 19, 2023
fd8b104
Merge branch 'master' into 18977-starlark-from-java-build-options
iancha1992 Jul 19, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion ...ain/java/com/google/devtools/build/lib/query2/cquery/StarlarkOutputFormatterCallback.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ public Object buildOptions(ConfiguredTarget target) {

// Add Starlark options.
for (Map.Entry<Label, Object> e : buildOptions.getStarlarkOptions().entrySet()) {
result.put(e.getKey().toString(), e.getValue());
result.put(e.getKey().toString(), Starlark.fromJava(e.getValue(), null));
}
return result.buildOrThrow();
}
Expand Down
34 changes: 28 additions & 6 deletions src/test/shell/integration/configured_query_test.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -952,9 +952,18 @@ bool_flag = rule(
build_setting = config.bool(flag = True),
)

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

list_flag = rule(
implementation = _list_flag_impl,
build_setting = config.string_list(flag = True),
)

def _dep_transition_impl(settings, attr):
return {
"//$pkg:myflag": True,
"//$pkg:mylistflag": ["a", "b"],
"//command_line_option:platform_suffix": "blah"
}

Expand All @@ -963,6 +972,7 @@ _dep_transition = transition(
inputs = [],
outputs = [
"//$pkg:myflag",
"//$pkg:mylistflag",
"//command_line_option:platform_suffix",
],
)
Expand All @@ -980,7 +990,7 @@ root_rule = rule(
EOF

cat > $pkg/BUILD <<'EOF'
load(":rules.bzl", "bool_flag", "root_rule")
load(":rules.bzl", "bool_flag", "list_flag", "root_rule")

exports_files(["rules.bzl"])

Expand All @@ -989,6 +999,11 @@ bool_flag(
build_setting_default = False,
)

list_flag(
name = "mylistflag",
build_setting_default = ["c"],
)

py_library(
name = "bar",
srcs = ["pylib.py"],
Expand All @@ -1008,24 +1023,31 @@ def format(target):
return str(target.label) + '%None'
first = str(bo['//command_line_option:platform_suffix'])
second = str(('//$pkg:myflag' in bo) and bo['//$pkg:myflag'])
return str(target.label) + '%' + first + '%' + second
third = str(bo['//$pkg:mylistflag'] if '//$pkg:mylistflag' in bo else None)
return str(target.label) + '%' + first + '%' + second + '%' + third
EOF

bazel cquery "//$pkg:bar" --output=starlark \
--starlark:file=$pkg/expr.star > output 2>"$TEST_log" || fail "Expected success"

assert_contains "//$pkg:bar%None%False" output
assert_contains "//$pkg:bar%None%False%None" output
Copy link

@Colecf Colecf Jul 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this None instead of the build_setting_default of ["c"]? This isn't necessarily something that needs to be changed if this is the behavior we want. I guess this does allow differentiating between an unset value and a value set to the default.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a consequence of how Starlark options are implemented: When they haven't been set anywhere, they aren't tracked and the dict of all options just won't contain any value for them, not even the default.

This makes sense given the decentralized nature of Bazel: Knowing all Starlark options and their default values would require loading all packages (and thus all repos) eagerly.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, that does make some sense, but then it is weird how then you can get the actual value when its set despite bar having no dependency on mylistflag.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's why build_options is "magic": It gives you access to the full internal view of all options that Bazel maintains. This has an entry for mylistflag simply because it has been set on the command-line. build_options doesn't care about dependencies here, which is why it is only made available in cquery, not in regular Starlark.


bazel cquery "//$pkg:bar" --output=starlark \
--//$pkg:myflag=True --//$pkg:mylistflag=c,d \
--starlark:file=$pkg/expr.star > output 2>"$TEST_log" || fail "Expected success"

assert_contains "//$pkg:bar%None%True%\\[\"c\", \"d\"]" output

bazel cquery "//$pkg:foo" --output=starlark \
--starlark:file=$pkg/expr.star > output 2>"$TEST_log" || fail "Expected success"

assert_contains "//$pkg:foo%None%False" output
assert_contains "//$pkg:foo%None%False%None" output

bazel cquery "kind(rule, deps(//$pkg:foo))" --output=starlark \
--starlark:file=$pkg/expr.star > output 2>"$TEST_log" || fail "Expected success"

assert_contains "//$pkg:foo%None%False" output
assert_contains "//$pkg:bar%blah%True" output
assert_contains "//$pkg:foo%None%False%None" output
assert_contains "//$pkg:bar%blah%True%\\[\"a\", \"b\"]" output

bazel cquery "//$pkg:rules.bzl" --output=starlark \
--starlark:file=$pkg/expr.star > output 2>"$TEST_log" || fail "Expected success"
Expand Down
Loading