Add --incompatible_eagerly_resolve_select_keys
#26216
Conversation
--incompatible_eagerly_resolve_select_keys--incompatible_resolve_select_keys_eagerly
187c6d1 to
e46e8dc
Compare
--incompatible_resolve_select_keys_eagerly--incompatible_eagerly_resolve_select_keys
e46e8dc to
5e6ea48
Compare
|
@bazel-io fork 8.3.0 |
|
I didn't have time to look at the code yet. The described behavior change seems reasonable at first glance, but I would like @brandjon to take a look and verify for gotchas. Also, this this change will require documentation updates (can be in followup PRs). |
|
Could you add a sentence or two as RELNOTES? |
3202026 to
1de426a
Compare
Done!
I can add it in a separate PR. Should I make the docs conditional on the flag value or only change them after the flip? |
|
I filed #26281. |
|
I agree with the spirit of this change, i.e. having strings canonicalized into label objects at the point where they're passed to One problem might be
Usually we include the effect of a flag in the documentation of the API that it affects. So the same PR that introduces the flag would also update the user docstrings for |
src/test/java/com/google/devtools/build/lib/packages/SelectTest.java
Outdated
Show resolved
Hide resolved
| packageContext != null | ||
| ? Label.parseWithPackageContext( | ||
| labelString, packageContext, repoMappingRecorder) | ||
| : labelString, |
There was a problem hiding this comment.
I'm a bit disappointed that we're adding complexity to the select machinery without getting the benefit of ensuring that SelectorValues hold Label keys in all cases, which would perhaps be progress toward merging this class into BuildType.Selector in the future. Is there any fundamental reason we can't normalize to Labels even when there is no packageContext / repoMappingRecorder?
There was a problem hiding this comment.
I think I got this now, thanks for pushing me. It also avoids explicit references to repoMappingRecorder.
I think that this problem is mostly orthogonal to this change, but I sent #26290 to fix these issues. Since selects aren't inspectable (unless you parse the string representation), it doesn't seem like much of a breaking change to preserve |
|
I also added doc updates, PTAL. |
|
I have no feedback beyond the existing conversation: particularly @brandjon 's approval. |
|
@brandjon Friendly ping |
|
@brandjon @gregestren Friendly ping |
|
@brandjon @gregestren Friendly ping |
gregestren
left a comment
There was a problem hiding this comment.
other macros that generate config_settings
Sorry, can you clarify this part? It's not just a macro generating a config_setting, but including a select(), isn't it?
src/main/java/com/google/devtools/build/lib/packages/SelectorList.java
Outdated
Show resolved
Hide resolved
a9cf114 to
c57fbbb
Compare
|
LGTM but what are the recent commits & failing tests? |
# Conflicts: # src/main/java/com/google/devtools/build/lib/packages/semantics/BuildLanguageOptions.java
c57fbbb to
f41e591
Compare
|
@gregestren I rebased onto master and didn't notice the build failure that caused. Should be fixed now. |
This avoids a common gotcha in which a `select` in a macro with repo names in string keys will generally not work correctly when loaded in the context of a different Bazel (Bzlmod) module that uses a different set of apparent names. In the rare case where resolution relative to the loading `BUILD` file is necessary (e.g. in Skylib's `selects.config_setting_group` or other macros that generate `config_setting`s and `select`s referring to them), the keys can be preprocessed with `native.package_relative_label`. RELNOTES: With the new `--incompatible_eagerly_resolve_select_keys` flag, the label string keys of `select` dicts in `.bzl` files are resolved relative to the containing file instead of relative to the BUILD file that ends up using the `select`. Use `native.package_relative_label` if this is not desired. Closes bazelbuild#26216. PiperOrigin-RevId: 811342967 Change-Id: I8a806ae9b94db248bcb18460984f6b418a1284d6
Cherry-pick of 054536e: This avoids a common gotcha in which a `select` in a macro with repo names in string keys will generally not work correctly when loaded in the context of a different Bazel (Bzlmod) module that uses a different set of apparent names. In the rare case where resolution relative to the loading `BUILD` file is necessary (e.g. in Skylib's `selects.config_setting_group` or other macros that generate `config_setting`s and `select`s referring to them), the keys can be preprocessed with `native.package_relative_label`. RELNOTES: With the new `--incompatible_eagerly_resolve_select_keys` flag, the label string keys of `select` dicts in `.bzl` files are resolved relative to the containing file instead of relative to the BUILD file that ends up using the `select`. Use `native.package_relative_label` if this is not desired. For #26216. Closes #26223 (comment) --------- Co-authored-by: Fabian Meumertzheim <fabian@meumertzhe.im>
This avoids a common gotcha in which a
selectin a macro with repo names in string keys will generally not work correctly when loaded in the context of a different Bazel (Bzlmod) module that uses a different set of apparent names.In the rare case where resolution relative to the loading
BUILDfile is necessary (e.g. in Skylib'sselects.config_setting_groupor other macros that generateconfig_settings andselects referring to them), the keys can be preprocessed withnative.package_relative_label.RELNOTES: With the new
--incompatible_eagerly_resolve_select_keysflag, the label string keys ofselectdicts in.bzlfiles are resolved relative to the containing file instead of relative to the BUILD file that ends up using theselect. Usenative.package_relative_labelif this is not desired.