bazelbuild
Add `--incompatible_eagerly_resolve_select_keys`
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_settings), 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.
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?
Done!
Also, this this change will require documentation updates (can be in followup PRs).
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 agree with the spirit of this change, i.e. having strings canonicalized into label objects at the point where they're passed to select(). This could simplify some logic and be more consistent with user expectations.
One problem might be native.existing_rules(), where due to user convenience and legacy, labels are represented as strings in the starlarkifyValue() conversion. This would mean that round-tripping a select() through this feature would potentially re-resolve the string in the context of the macro rather than preserve its original value. This is more an indictment of the existing_rules API than of your change, but do you have a plan for how to handle this? (E.g., is it a non-issue because it's perhaps rare, or already broken in practice? Or is it a breaking change worth pushing through?)
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?
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 select() or the APIs that read it (e.g. native.existing_rules()). (It might be acceptable to only document the flag itself rather than the Starlark APIs if the existing Starlark API docs don't specify the type.)
One problem might be
native.existing_rules(), where due to user convenience and legacy, labels are represented as strings in thestarlarkifyValue()conversion. This would mean that round-tripping aselect()through this feature would potentially re-resolve the string in the context of the macro rather than preserve its original value. This is more an indictment of theexisting_rulesAPI than of your change, but do you have a plan for how to handle this? (E.g., is it a non-issue because it's perhaps rare, or already broken in practice? Or is it a breaking change worth pushing through?)
I think that this problem is mostly orthogonal to this change, but I sent https://github.com/bazelbuild/bazel/pull/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 Labels in keys, so I did that.
I have no feedback beyond the existing conversation: particularly @brandjon 's approval.
@gregestren I rebased onto master and didn't notice the build failure that caused. Should be fixed now.