#development
Title
w
witty-crayon-22786
10/24/2021, 4:48 PMre: RuleRunner not including environment vars by default:
test isolation means that only environment vars that have been explicitly included are available anyway, so in pantsbuild/pants, that’s https://github.com/pantsbuild/pants/blob/18c1468462377b79cccf0685f02491c0a25f6402/pants.toml#L119-L129
but we then also do not include env vars in RuleRunner by default, so any tests that want to use those need to include them explicitly via
env_inheritthe effect is that hundreds of tests use
set_options(.., env_inherit=..)run_goal_rule(.., env_inherit=..)and https://github.com/pantsbuild/pants/pull/13340 makes it even more widespread: in order to use Python to extract the
gzso: i think that we should loosen this a bit, probably by either:
1. inheriting the entire env by default
2. inheriting a relevant subset of the env by default
3. EDIT: adding dedicated methods for the commonly inherited variables
given that we already filter the env for tests, i’d lean toward (1).
c
curved-television-6568
10/24/2021, 5:21 PMIsn't it those three vars in particular that needs to be inherited frequently? Perhaps have a dedicated constructor or so that sets them by default, so it still will be opt in, but less verbose, and also DRY (in case of more vars needed in the future etc)
w
witty-crayon-22786
10/24/2021, 5:24 PMmaybe. a challenge though is that all of
__new__set_optionsrun_goal_rule👍 1
c
curved-television-6568
10/24/2021, 5:26 PMAh..
h
hundreds-father-404
10/24/2021, 5:45 PMI think that makes sense. We should probably also be using the field for env vars rather than global option?
h
happy-kitchen-89482
10/24/2021, 8:56 PMMakes sense to me.
w
witty-crayon-22786
10/25/2021, 4:39 PMsorry, were folks leaning toward 1, 2, or the newly added 3 ?
h
happy-kitchen-89482
10/25/2021, 4:44 PMI was leaning toward 1
👍 1
w
witty-crayon-22786
10/25/2021, 6:25 PM@hundreds-father-404, @curved-television-6568: any objections? i’m thinking that explicitly specifying an environment would override the current env, rather than merging (similar to
popen…you know what though. this might be a yak too far on my current yakshave. might leave a TODO pointed to a ticket for this one.
👍 2
c
curved-television-6568
10/25/2021, 6:28 PMWas just going to reply that, if the default would be set a side if you provide env options with RuleRunner.set_options(), then that sgtm. :)
w
witty-crayon-22786
10/25/2021, 7:17 PM