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

Adding expansion.bzl for Fully Expanding Environment Variables #486

Open
wants to merge 17 commits into
base: main
Choose a base branch
from

Commits on Jan 23, 2024

  1. Adding expansion.bzl for Fully Expanding Environment Variables

    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 committed Jan 23, 2024
    Configuration menu
    Copy the full SHA
    d6ce59b View commit details
    Browse the repository at this point in the history
  2. * Adding bzl_library target for expansion.bzl in lib/BUILD.

    * Making `buildifier` happy.
    CauhxMilloy committed Jan 23, 2024
    Configuration menu
    Copy the full SHA
    d724543 View commit details
    Browse the repository at this point in the history
  3. * Replacing platform dependent strings (from $(location ...) and si…

    …milar) for consistent assertions.
    CauhxMilloy committed Jan 23, 2024
    Configuration menu
    Copy the full SHA
    587f254 View commit details
    Browse the repository at this point in the history
  4. Configuration menu
    Copy the full SHA
    416305e View commit details
    Browse the repository at this point in the history
  5. Configuration menu
    Copy the full SHA
    789a32f View commit details
    Browse the repository at this point in the history

Commits on Jan 24, 2024

  1. * Reducing some duplication (merging _expand_tc_all_keys_in_str() a…

    …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.
    CauhxMilloy committed Jan 24, 2024
    Configuration menu
    Copy the full SHA
    c38d3e8 View commit details
    Browse the repository at this point in the history

Commits on Jan 28, 2024

  1. * Adding validation logic.

      * 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 committed Jan 28, 2024
    Configuration menu
    Copy the full SHA
    a823ff7 View commit details
    Browse the repository at this point in the history
  2. Configuration menu
    Copy the full SHA
    2ad568f View commit details
    Browse the repository at this point in the history

Commits on Feb 18, 2024

  1. Configuration menu
    Copy the full SHA
    9e4779a View commit details
    Browse the repository at this point in the history

Commits on May 11, 2024

  1. Configuration menu
    Copy the full SHA
    a23dbc4 View commit details
    Browse the repository at this point in the history
  2. Configuration menu
    Copy the full SHA
    f0cbec4 View commit details
    Browse the repository at this point in the history
  3. * Updating expected rlocation value to work with different versions o…

    …f bazel used in testing.
    CauhxMilloy committed May 11, 2024
    Configuration menu
    Copy the full SHA
    4fd0388 View commit details
    Browse the repository at this point in the history
  4. Configuration menu
    Copy the full SHA
    7e24e95 View commit details
    Browse the repository at this point in the history
  5. * Downgrading usage of dict | dict in tests/expansion_tests.bzl t…

    …o use `dict(dict).update(dict)` to be backwards compatible with Bazel 5.x.
    CauhxMilloy committed May 11, 2024
    Configuration menu
    Copy the full SHA
    a2ff961 View commit details
    Browse the repository at this point in the history

Commits on May 12, 2024

  1. Configuration menu
    Copy the full SHA
    aa133b9 View commit details
    Browse the repository at this point in the history
  2. * Fixing formatting.

    CauhxMilloy committed May 12, 2024
    Configuration menu
    Copy the full SHA
    826b30c View commit details
    Browse the repository at this point in the history
  3. * Downgrading usage of dict | dict to use dict(dict, **dict) to b…

    …e backwards compatible with Bazel 5.x.
    
      * `dict(dict).update(dict)` is not a good replacement as it returns `NoneType`.
    CauhxMilloy committed May 12, 2024
    Configuration menu
    Copy the full SHA
    e5d9112 View commit details
    Browse the repository at this point in the history