Adding expansion.bzl for Fully Expanding Environment Variables
#486
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The built-in functionality (exposed Skylark methods on `ctx`) for expanding environment variables leaves a lot of implementation to do in `bzl` files. This PR adds in that missing functionality for easy reuse with a new `expansion` struct. `expansion` has various methods for expanding environment variables with a flexible API. The existing APIs handle only one piece at a time: * `ctx.expand_location()` only handles `$(location ...)` (and similar) expansion. It does not handle any "make variable" expansion (no expansion from `TemplateVariableInfo` or other providers via toolchains). * `ctx.expand_make_variables()` only handles make variables. If it encounters `$(location ...)` (or similar), it [errors out with no means of recovery](https://github.com/bazelbuild/bazel/blob/addf41bce1f26e8bc4bd0b09cb2fbffbb0446f25/src/main/java/com/google/devtools/build/lib/analysis/ConfigurationMakeVariableContext.java#L136). This method is also marked as deprecated, with `ctx.var` listed as the preferred API. * `ctx.var` is a simple dictionary, which contains resolved mappings based on toolchains. However, being a simple data structure (not a function) leaves recursive functionality and/or integration with `ctx.expand_location()` to be implemented by hand (or in this PR). Many internal systems make use of functionality that fully resolves both make variables and `$(location ...)` (and does so recursively). This is done with `ruleContext.getExpander().withDataLocations()` ([example](https://github.com/bazelbuild/bazel/blob/addf41bce1f26e8bc4bd0b09cb2fbffbb0446f25/src/main/java/com/google/devtools/build/lib/analysis/RunfilesSupport.java#L522)). However, this is never exposed to skylark. This means that built-in rules will have the `env` attribute fully expanded, but custom rules cannot (easily). The above mentioned methods have their own (somewhat duplicated) implementations ([`ctx.expand_location()`](https://github.com/bazelbuild/bazel/blob/addf41bce1f26e8bc4bd0b09cb2fbffbb0446f25/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleContext.java#L1011), [`ctx.expand_make_variables()`](https://github.com/bazelbuild/bazel/blob/addf41bce1f26e8bc4bd0b09cb2fbffbb0446f25/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleContext.java#L964), [`ctx.var`](https://github.com/bazelbuild/bazel/blob/addf41bce1f26e8bc4bd0b09cb2fbffbb0446f25/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleContext.java#L823)). Note the identical `ConfigurationMakeVariableContext` initialization [here](https://github.com/bazelbuild/bazel/blob/addf41bce1f26e8bc4bd0b09cb2fbffbb0446f25/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleContext.java#L949) and [here](https://github.com/bazelbuild/bazel/blob/36fa60b1805faa7da2c4b5330b4b186740f5f00d/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java#L978) -- identical except for the use of `additionalSubstitutionsMap`, which could have been delegated (similar to the impl of `LocationTemplateContext`). Also note the separate/duplicated parsing implementations in [`LocationTemplateContext`](https://github.com/bazelbuild/bazel/blob/addf41bce1f26e8bc4bd0b09cb2fbffbb0446f25/src/main/java/com/google/devtools/build/lib/analysis/stringtemplate/TemplateExpander.java#L77) and in [`LocationExpander`](https://github.com/bazelbuild/bazel/blob/36fa60b1805faa7da2c4b5330b4b186740f5f00d/src/main/java/com/google/devtools/build/lib/analysis/LocationExpander.java#L179). This PR tries to avoid more duplicate implementations (which can't really happen anyway; as this is in external Skylark, not Java / Bazel runtime). These new methods make use of `ctx.expand_location()` and `ctx.var` to allow fully recursive variable expansion that incorporates both inputs (toolchains and `location`). The methods avoid copies/string mutations/extra loops when necessary to ensure that the functions can run quickly. The methods' API allows for manual/direct data structures (lists/dicts) as input, or for pulling values directly from `ctx`. `tests/expansion_tests.bzl` added to test all added methods. This PR is being submitted here so that it can be reused (instead of copied in many repos). It is also preferable to add functionality here in Skylib, instead of (or in addition to) any changes in the [Bazel repo](https://github.com/bazelbuild/bazel) so that it can be more easily pulled into projects with a pinned Bazel version. Please feel free to leave comments or suggestion on ways to improve this PR, or let me know if you have any questions. Thanks!
CauhxMilloy
requested review from
brandjon,
tetromino,
comius and
c-mita
as code owners
* Making `buildifier` happy.
…milar) for consistent assertions.
…nd `_expand_tc_and_loc_all_keys_in_str()` into `_expand_all_keys_in_str()` with None-able arg). * Updating handling / init of optional `additional_lookup_dict` arg. * Cleaning up some doc comments. * Updating tests. * Adding demonstration where `additional_lookup_dict` overrides value from toolchains. * Adding demonstration where recursive expansion involves env dict and toolchain dict back and forth. * Adding a few more section separator comments.
* Adding `_validate_all_keys_expanded()` to validate there are no expandable keys in a given string.
* Will either call `fail()` or return list of failures (depending on `fail_instead_of_return` parameter).
* Failure messages will contain the parsed unexpanded variable and the whole string which contains it.
* Adding optional `validate_expansion` parameter to allow automatic validation after the expansion process.
* Adding exposed `validate_expansions` to allow the client to call the functionality directly.
* Adding new tests for validation logic.
* Parameterized over many different configurations.
* All tests pass in <= 0.1s each.
* Pulling out `_CONSIDERED_KEY_FORMATS` into file-scoped constant.
* Pulling out "odd count dollar sign" logic into `_odd_count_dollar_sign_repeat()`.
* Updating some variable names and adding more comments.
CauhxMilloy
added a commit
to CauhxMilloy/bazel-bats
that referenced
this pull request
Jan 29, 2024
* Adding `expansion.bzl` directly to this project. * Allows for better environment variable expansion than built-in skylark methods. * I'm also trying to submit this to Skylib (bazelbuild/bazel-skylib#486). * I could add this here (instead or in addition to). * Obviously, (also) adding this to Skylib would be more reusable for more repos. * Once submitted to Skylib (if accepted), this addition of `expansion.bzl` could be replaced with referencing the one in Skylib. * Doing this would add an extra dependency for bazel-bats (needing to pull in Skylib in `bazel_bats_dependencies()`). * Alternatively, could just keep this file here (for no extra dependency). * Updating `env` attribute expansion logic in `_bats_test_impl()`. * Allows better environment expansion logic than previously used. * Updating `tests/hello_world.bats` to validate many different environment variable expansion configurations.
CauhxMilloy
added a commit
to CauhxMilloy/bazel-bats
that referenced
this pull request
Jan 29, 2024
This is one approach for improving `bats_test` environment variable expansion logic (to get at least parity with built-in expansion logic). See https://github.com/CauhxMilloy/bazel-bats/tree/improvedExpansionWithShTest for `sh_test` approach. * Adding `expansion.bzl` directly to this project. * Allows for better environment variable expansion than built-in skylark methods. * I'm also trying to submit this to Skylib (bazelbuild/bazel-skylib#486). * I could add this here (instead or in addition to). * Obviously, (also) adding this to Skylib would be more reusable for more repos. * Once submitted to Skylib (if accepted), this addition of `expansion.bzl` could be replaced with referencing the one in Skylib. * Doing this would add an extra dependency for bazel-bats (needing to pull in Skylib in `bazel_bats_dependencies()`). * Alternatively, could just keep this file here (for no extra dependency). * Updating `env` attribute expansion logic in `_bats_test_impl()`. * Allows better environment expansion logic than previously used. * Updating `tests/hello_world.bats` to validate many different environment variable expansion configurations.
CauhxMilloy
added a commit
to CauhxMilloy/bazel-bats
that referenced
this pull request
Jan 30, 2024
* Adding `expansion.bzl` directly to this project. * Allows for better environment variable expansion than built-in skylark methods. * I'm also trying to submit this to Skylib (bazelbuild/bazel-skylib#486). * I could add this here (instead or in addition to). * Obviously, (also) adding this to Skylib would be more reusable for more repos. * Once submitted to Skylib (if accepted), this addition of `expansion.bzl` could be replaced with referencing the one in Skylib. * Doing this would add an extra dependency for bazel-bats (needing to pull in Skylib in `bazel_bats_dependencies()`). * Alternatively, could just keep this file here (for no extra dependency). * Updating `env` attribute expansion logic in `_bats_test_impl()`. * Allows better environment expansion logic than previously used. * Updating `tests/hello_world.bats` to validate many different environment variable expansion configurations. This is one approach for improving `bats_test` environment variable expansion logic (to get at least parity with built-in expansion logic). See https://github.com/CauhxMilloy/bazel-bats/tree/improvedExpansionWithShTest for `sh_test` approach.
CauhxMilloy
added a commit
to CauhxMilloy/bazel-bats
that referenced
this pull request
Jan 30, 2024
* Adding `expansion.bzl` directly to this project. * Allows for better environment variable expansion than built-in skylark methods. * I'm also trying to submit this to Skylib (bazelbuild/bazel-skylib#486). * I could add this here (instead or in addition to). * Obviously, (also) adding this to Skylib would be more reusable for more repos. * Once submitted to Skylib (if accepted), this addition of `expansion.bzl` could be replaced with referencing the one in Skylib. * Doing this would add an extra dependency for bazel-bats (needing to pull in Skylib in `bazel_bats_dependencies()`). * Alternatively, could just keep this file here (for no extra dependency). * Updating `env` attribute expansion logic in `_bats_test_impl()`. * Allows better environment expansion logic than previously used. * Updating `tests/hello_world.bats` to validate many different environment variable expansion configurations. This is one approach for improving `bats_test` environment variable expansion logic (to get at least parity with built-in expansion logic). See https://github.com/CauhxMilloy/bazel-bats/tree/improvedExpansionWithShTest for `sh_test` approach.
CauhxMilloy
added a commit
to CauhxMilloy/bazel-bats
that referenced
this pull request
Jan 30, 2024
* Adding `expansion.bzl` directly to this project. * Allows for better environment variable expansion than built-in skylark methods. * I'm also trying to submit this to Skylib (bazelbuild/bazel-skylib#486). * I could add this here (instead or in addition to). * Obviously, (also) adding this to Skylib would be more reusable for more repos. * Once submitted to Skylib (if accepted), this addition of `expansion.bzl` could be replaced with referencing the one in Skylib. * Doing this would add an extra dependency for bazel-bats (needing to pull in Skylib in `bazel_bats_dependencies()`). * Alternatively, could just keep this file here (for no extra dependency). * Updating `env` attribute expansion logic in `_bats_test_impl()`. * Allows better environment expansion logic than previously used. * Updating `tests/hello_world.bats` to validate many different environment variable expansion configurations. This is one approach for improving `bats_test` environment variable expansion logic (to get at least parity with built-in expansion logic). See https://github.com/CauhxMilloy/bazel-bats/tree/improvedExpansionWithShTest for `sh_test` approach.
filmil
pushed a commit
to filmil/bazel-bats
that referenced
this pull request
Feb 3, 2024
* Adding `expansion.bzl` directly to this project. * Allows for better environment variable expansion than built-in skylark methods. * I'm also trying to submit this to Skylib (bazelbuild/bazel-skylib#486). * I could add this here (instead or in addition to). * Obviously, (also) adding this to Skylib would be more reusable for more repos. * Once submitted to Skylib (if accepted), this addition of `expansion.bzl` could be replaced with referencing the one in Skylib. * Doing this would add an extra dependency for bazel-bats (needing to pull in Skylib in `bazel_bats_dependencies()`). * Alternatively, could just keep this file here (for no extra dependency). * Updating `env` attribute expansion logic in `_bats_test_impl()`. * Allows better environment expansion logic than previously used. * Updating `tests/hello_world.bats` to validate many different environment variable expansion configurations. This is one approach for improving `bats_test` environment variable expansion logic (to get at least parity with built-in expansion logic). See https://github.com/CauhxMilloy/bazel-bats/tree/improvedExpansionWithShTest for `sh_test` approach.
…f bazel used in testing.
…o use `dict(dict).update(dict)` to be backwards compatible with Bazel 5.x.
…e backwards compatible with Bazel 5.x. * `dict(dict).update(dict)` is not a good replacement as it returns `NoneType`.
* Renaming all `expand_with_*` functions to `expand_dict_strings_with_*`.
* Explicit that it acts on a dict of strings.
* Adding `expand_list_strings_with_manual_dict` and `expand_list_strings_with_manual_dict_and_location` functions.
* Similar to the existing `expand_dict_strings_with_manual_dict*` functions, these expand values but from a list of strings instead of dict of strings.
* Note that these methods only perform recursion via the "manual dict" (the input datatype, `list`, is not an associative type).
* Simpler API for just expanding a few string values (of any attr type, or not necessarily associated with any attr).
* Renaming `expand_location` parameter(s) to `wrapped_expand_location`.
* This makes it more clear that this is not `ctx.expand_location` directly, but rather a wrapped version of that function.
* Adding `wrap_expand_location` method.
* Offers a helper function for creating `wrapped_expand_location` (used by many expansion functions) from a given `ctx` and `deps`.
* Updating doc string comments.
* Updating `tests/bzl/expansion_tests.bzl`.
* Renaming all `expand_with_*` tests to `expand_dict_strings_with_*`.
* Adding `expand_list_strings_with_*` tests (for new functions).
* Adding "no recursion" "expected resolved" dicts for new tests.
* Adding `TOOLCHAIN_INDIRECT_ENV_VAR*` values to expand, which show recursion specifically among only toolchain values.
|
Wow, this is one nice PR. It's a shame it's here for 1.5 years. |
This was referenced Jul 12, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The built-in functionality (exposed Skylark methods on
ctx) for expanding environment variables leaves a lot of implementation to do inbzlfiles. This PR adds in that missing functionality for easy reuse with a newexpansionstruct.expansionhas various methods for expanding environment variables with a flexible API.The existing APIs handle only one piece at a time:
ctx.expand_location()only handles$(location ...)(and similar) expansion. It does not handle any "make variable" expansion (no expansion fromTemplateVariableInfoor other providers via toolchains).ctx.expand_make_variables()only handles make variables. If it encounters$(location ...)(or similar), it errors out with no means of recovery. This method is also marked as deprecated, withctx.varlisted as the preferred API.ctx.varis a simple dictionary, which contains resolved mappings based on toolchains. However, being a simple data structure (not a function) leaves recursive functionality and/or integration withctx.expand_location()to be implemented by hand (or in this PR).Many internal systems make use of functionality that fully resolves both make variables and
$(location ...)(and does so recursively). This is done withruleContext.getExpander().withDataLocations()(example). However, this is never exposed to skylark. This means that built-in rules will have theenvattribute fully expanded, but custom rules cannot (easily).The above mentioned methods have their own (somewhat duplicated) implementations (
ctx.expand_location(),ctx.expand_make_variables(),ctx.var). Note the identicalConfigurationMakeVariableContextinitialization here and here -- identical except for the use ofadditionalSubstitutionsMap, which could have been delegated (similar to the impl ofLocationTemplateContext). Also note the separate/duplicated parsing implementations inLocationTemplateContextand inLocationExpander.This PR tries to avoid more duplicate implementations (which can't really happen anyway; as this is in external Skylark, not Java / Bazel runtime). There is one extra implementation to mention (in
cc_helper), used bycc_binary(). This is referenced in Skylark here, and implemented here. This implementation is also not consistent with the above listed Java-based implementations and it is also not accessible/exposed to otherbzlfiles (can't accesscc_helperfrom@rules_cc//). This implementation is done differently, as to allow for all the listed functionality.PR details:
ctx.expand_location()andctx.varto allow fully recursive variable expansion that incorporates both inputs (toolchains andlocation).ctx.$VAR,$(VAR), and${VAR}for variable expansion.envdict for recursion (not only checkinglocation/toolchains).tests/expansion_tests.bzladded to test all added methods.This PR is being submitted here so that it can be reused (instead of copied in many repos). It is also preferable to add functionality here in Skylib, instead of (or in addition to) any changes in the Bazel repo so that it can be more easily pulled into projects with a pinned Bazel version.
Please feel free to leave comments or suggestion on ways to improve this PR, or let me know if you have any questions. Thanks!