https://pantsbuild.org/ logo
Join Slack
Powered by
<@UB2J9BQA0>: i’d like to start building atop <htt...
# development
w
@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
I'm back
👍 1
w
thanks, i’m unblocked.
h
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
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
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
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
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
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
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
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
Ah, okay
I can start working on modeling for 
docker_environment
? Specifically how users will specify the image
w
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
yeah, and I'm not certain 
EnvironmentName
makes sense - I'm proposing we should confirm the name & import path now
w
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
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
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
would you recommend the option be called 
[environments].names
? I think I like that more than 
aliases
🙂
w
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
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
yea, possibly.
but yes, i think that “environment name” is more direct, but don’t feel strongly about it.
h
yeah I'm +1 for 
name
rather than 
alias
. We use "resolve name" everywhere also
w
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
I don't follow that point
w
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
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
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
Open in Slack
PreviousNext
Powered by