https://pantsbuild.org/ logo
Join Slack
Powered by
re: RuleRunner not including environment vars by d...
# development
w
re: 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_inherit
the effect is that hundreds of tests use 
set_options(.., env_inherit=..)
or 
run_goal_rule(.., env_inherit=..)
to include those vars
and https://github.com/pantsbuild/pants/pull/13340 makes it even more widespread: in order to use Python to extract the 
gz
of coursier, all of the JVM tests will need to set those soon too.
so: 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
Isn'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
maybe. a challenge though is that all of 
__new__
, 
set_options
and 
run_goal_rule
currently “set” the env, so you’d need dedicated methods for each of those. and as mentioned, it’s hundreds of callsites.
👍 1
c
Ah..
h
I think that makes sense. We should probably also be using the field for env vars rather than global option?
h
Makes sense to me.
w
sorry, were folks leaning toward 1, 2, or the newly added 3 ?
h
I was leaning toward 1
👍 1
w
@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
i suppose)
…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
Was 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
Open in Slack
PreviousNext
Powered by