https://pantsbuild.org/ logo
#development
Title
# development
w

witty-crayon-22786

10/24/2021, 4:48 PM
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

curved-television-6568

10/24/2021, 5:21 PM
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

witty-crayon-22786

10/24/2021, 5:24 PM
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

curved-television-6568

10/24/2021, 5:26 PM
Ah..
h

hundreds-father-404

10/24/2021, 5:45 PM
I 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 PM
Makes sense to me.
w

witty-crayon-22786

10/25/2021, 4:39 PM
sorry, were folks leaning toward 1, 2, or the newly added 3 ?
h

happy-kitchen-89482

10/25/2021, 4:44 PM
I 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
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

curved-television-6568

10/25/2021, 6:28 PM
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

witty-crayon-22786

10/25/2021, 7:17 PM