https://pantsbuild.org/ logo
w

witty-crayon-22786

08/31/2022, 5:57 PM
@hundreds-father-404: i’d like to start building atop https://github.com/pantsbuild/pants/pull/16714 once the naming is settled: otherwise my first commits in https://github.com/pantsbuild/pants/pull/16682 will probably be renaming it: fine either way, just let me know whether i should rebase on it as is, or wait for you to rename
ok, seems like you’re out, so i’ll go ahead and merge and do the rename later.
h

hundreds-father-404

08/31/2022, 7:21 PM
I'm back
👍 1
w

witty-crayon-22786

08/31/2022, 7:23 PM
thanks, i’m unblocked.
h

hundreds-father-404

08/31/2022, 7:23 PM
should we still tweak the names now? I'm uncertain I agree with your suggestions
@witty-crayon-22786 and should I continue with the PR to inject
ResolvedEnvironmentTarget
for every goal rule? Is that stopping on your shoes?
w

witty-crayon-22786

08/31/2022, 7:24 PM
i’ll need to use the
EnvironmentName
name in about 40 files, so it should be shorter. but it’s not blocking.
and should I continue with the PR to inject
ResolvedEnvironmentTarget
for every goal rule? Is that stopping on your shoes?
that is what i am currently working on
h

hundreds-father-404

08/31/2022, 7:27 PM
Ah okay, so then I can switch to the 2.12-2.14 releases? The other environment work is wiring up more options + some polish
w

witty-crayon-22786

08/31/2022, 7:28 PM
yea, sounds good
ok, everything is “working” here, although: 1. rule graph solving got about 50% slower (but i see a quick improvement to make there) 2. a boatload of
RuleRunner
tests need to be updated to provide an
EnvironmentName
👍 1
i’m brainstorming changes that could be made to RuleRunner to make this easier… unfortunately, it’s akin to the old
OptionsBootstrapper
parameter in that it touches basically every
QueryRule
and
product_request
h

hundreds-father-404

08/31/2022, 10:44 PM
a boatload of RuleRunner tests need to be updated to provide an EnvironmentName
lmk if you want help
it’s akin to the old OptionsBootstrapper parameter in that it touches basically every QueryRule and product_request
ah, rip
w

witty-crayon-22786

08/31/2022, 10:52 PM
i’m tempted to actually dig a little deeper on the rule graph side to allow the Environment to be conditionally installed as a singleton… currently that doesn’t work because the new @union syntax from https://github.com/pantsbuild/pants/pull/16717 requires that the type is already in scope, and doesn’t try to solve for it.
then the RuleRunner could be initialized with a singleton environment (by default)
h

hundreds-father-404

09/01/2022, 4:08 PM
Hey gm. I think we can split out your PR into two? One that solely provides the
ResolvedEnvironmentAlias
, and another that makes the changes to
Platform
? Specifically, I want to get
python_bootstrap.py
so it simply requests
ResolvedEnvironmentTarget
in its rule signature, vs. right now having to use
Get()
w

witty-crayon-22786

09/01/2022, 4:19 PM
possibly. the one you’re waiting on is the one that will take longer to land though, so i don’t think that that will unblock you sooner
having the
EnvironmentName
injected everywhere is the bit that has all of the test fallout
h

hundreds-father-404

09/01/2022, 4:20 PM
Ah, okay
I can start working on modeling for
docker_environment
? Specifically how users will specify the image
w

witty-crayon-22786

09/01/2022, 4:21 PM
yea, sounds good.
my target is still to land https://github.com/pantsbuild/pants/pull/16717 and https://github.com/pantsbuild/pants/pull/16721 independently… but i want the latter one green before focusing on landing the first, because it’s involving rule graph changes.
👍 1
ok, i believe that i’ve gotten the “tests use a singleton environment” hack in place for https://github.com/pantsbuild/pants/pull/16721 … i’ll start cleaning up https://github.com/pantsbuild/pants/pull/16717 and then hopefully have them both reviewable by eod
spoke too soon. still poking at it.
👍 1
Ok, got it green. Will need some cleanup, but I expect that it will be good to land by noon tomorrow.
@hundreds-father-404: re https://github.com/pantsbuild/pants/pull/16721#discussion_r962023259 : the idea behind
EnvironmentName
would be to use a name that we think that we can commit to moving forward. i think that that should be a separate change from actually using/resolving and renaming
ResolvedEnvironmentAlias
… the primary thing to get right is just not needing to change the name again.
h

hundreds-father-404

09/02/2022, 10:01 PM
yeah, and I'm not certain
EnvironmentName
makes sense - I'm proposing we should confirm the name & import path now
w

witty-crayon-22786

09/02/2022, 10:02 PM
my feeling with using
pants.engine.environment.EnvironmentName
was that it was close to the ideal for such a widely used type.
something that is used in ~every API shouldn’t* be in
util_rules
, for example.
h

hundreds-father-404

09/02/2022, 10:02 PM
Thoughts on "name" vs "alias"? We can change the subsystem option still of course
something that is used in ~every API should be in util_rules, for example.
Meaning, this should be defined in util_rules or in engine?
w

witty-crayon-22786

09/02/2022, 10:03 PM
shouldn’t* sorry. will edit
Thoughts on “name” vs “alias”? We can change the subsystem option still of course
while it is possible to change the names at runtime (which feels closer to “aliasing” for me), in practice, that is the assigned name of the environment. i’m not sure it makes sense to have something called
EnvironmentAlias
unless you have already* something else called
EnvironmentName
h

hundreds-father-404

09/02/2022, 10:06 PM
would you recommend the option be called
[environments].names
? I think I like that more than
aliases
🙂
w

witty-crayon-22786

09/02/2022, 10:07 PM
a global dict option
environments
is what i had been imagining
Copy code
[GLOBAL]

[environments]
name1 = ..
name2 = ..
although maybe we need to future proof by putting it in a subsystem? just not sure what other options we’re likely to get there
h

hundreds-father-404

09/02/2022, 10:11 PM
just not sure what other options we’re likely to get there
I still think we may want
[environments].local_env
which allows you to force Pants to use a certain value. You can do that by changing the value for a particular name..but that's awkward with a dict option because you need to make sure all the previously defined names are still valid, or you'll get errors maybe even
--local-env
is just
: str
, and it's really intended for things like
pants.rc
. Rather than the more complex
DictOption
mapping Platform -> name
w

witty-crayon-22786

09/02/2022, 10:14 PM
yea, possibly.
but yes, i think that “environment name” is more direct, but don’t feel strongly about it.
h

hundreds-father-404

09/02/2022, 10:15 PM
yeah I'm +1 for
name
rather than
alias
. We use "resolve name" everywhere also
w

witty-crayon-22786

09/02/2022, 10:16 PM
ah, the other reason is that 99% of the time when users are interacting with this, it will be in BUILD files rather than in
pants.toml
h

hundreds-father-404

09/02/2022, 10:16 PM
I don't follow that point
w

witty-crayon-22786

09/02/2022, 10:16 PM
and even if you can “re-alias” something via an option, users will think of it as a name
1
as you said: similar to a resolve name
h

hundreds-father-404

09/02/2022, 10:17 PM
ah I get it. agreed
I'm still not certain on the location of the definition, and I'm not sure if import cycles will play a role here. The other import many files will have is the
EnvironmentField
w

witty-crayon-22786

09/02/2022, 10:19 PM
as mentioned in there, my thinking is that environment variable handling will end up moving out of this module
so i don’t think that it increases the risks of cycles per-se.
broke out the platform change: https://github.com/pantsbuild/pants/pull/16765