Skip to content

Commit

Permalink
Don't filter the env passed to OptionsBootstrapper. (#20956)
Browse files Browse the repository at this point in the history
The native parser needs access to all env vars for interpolation,
so we can't filter down to just PANTS_*.

Once we're rid of the legacy parser and can simplify options
bootstrapping, we can revisit exactly how to do this, but for
now we're OK with OptionsBootstrapper holding more data
than it strictly needs.
  • Loading branch information
benjyw authored May 27, 2024
1 parent 3b0ebf3 commit 57a230c
Show file tree
Hide file tree
Showing 2 changed files with 18 additions and 11 deletions.
14 changes: 9 additions & 5 deletions src/python/pants/option/options_bootstrapper.py
Original file line number Diff line number Diff line change
Expand Up @@ -184,11 +184,15 @@ def filecontent_for(path: str) -> FileContent:
bootstrap_option_values.pants_bin_name, get_buildroot()
)

env_tuples = tuple(
sorted(
(item for item in env.items() if item[0].startswith("PANTS_")),
)
)
# TODO: We really only need the env vars starting with PANTS_, plus any env
# vars used in env.FOO-style interpolation in config files.
# Filtering to just those would allow OptionsBootstrapper to have a less
# unwieldy __str__.
# We used to filter all but PANTS_* (https://github.com/pantsbuild/pants/pull/7312),
# but we can't now because of env interpolation in the native config file parser.
# We can revisit this once the legacy python parser is no more, and we refactor
# the OptionsBootstrapper and/or convert it to Rust.
env_tuples = tuple(sorted(env.items()))
return cls(
env_tuples=env_tuples,
bootstrap_args=bargs,
Expand Down
15 changes: 9 additions & 6 deletions src/python/pants/option/options_bootstrapper_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -146,14 +146,17 @@ def test_create_bootstrapped_options(self) -> None:
assert "/qux/baz" == opts.for_scope("foo").bar
assert "/pear/banana" == opts.for_scope("fruit").apple

def test_bootstrapped_options_ignore_irrelevant_env(self) -> None:
included = "PANTS_DISTDIR"
excluded = "NON_PANTS_ENV"
def test_bootstrapped_options_include_all_env(self) -> None:
pants_option = "PANTS_DISTDIR"
not_a_pants_option = "NON_PANTS_ENV"
bootstrapper = OptionsBootstrapper.create(
env={excluded: "pear", included: "banana"}, args=[], allow_pantsrc=False
env={not_a_pants_option: "pear", pants_option: "banana"}, args=[], allow_pantsrc=False
)
assert included in bootstrapper.env
assert excluded not in bootstrapper.env
assert pants_option in bootstrapper.env
# See https://github.com/pantsbuild/pants/pull/20956 for context.
# If we revisit and end up excluding env vars that aren't PANTS_* and aren't needed for
# interpolation in config, change this test to check that (and rename this test function).
assert not_a_pants_option in bootstrapper.env

def test_create_bootstrapped_multiple_pants_config_files(self) -> None:
"""When given multiple config files, the later files should take precedence when options
Expand Down

0 comments on commit 57a230c

Please sign in to comment.