pantsbuild #development

Channels

welcome

github-notifications

announce

busy-vase-39202

09/16/2022, 5:54 PM

Everyone who currently serves in the elected role of Pants Contributor or Pants Maintainer is a member of Pants Team, and are listed on https://www.pantsbuild.org/docs/team (if you spot an omission, ping me please 🙏). If you're one of the members of Pants Team, please fill out https://docs.google.com/forms/d/1BTBvdaaPkJ_2Fp7zkSFRvD0dCBXrZp58-_V7uTyGECE/ with your preferred email address for the calendar invites and low-volume (1-3 msgs/month) upcoming mailing list for future team announcements like this one.

ancient-vegetable-10556

09/16/2022, 6:18 PM

Conclusion: it makes sense (for the time being at least) for there to be a clear separation between Subsystems and environment targets. There’s a modelling issue that the default values thing shows — there are some cases where option parsing is necessary in contexts where environments don’t make sense (like, for example, default values for fields on targets which also need to specify an environment), and it doesn’t make good sense to make these bootstrap options

👍 1

ancient-vegetable-10556

09/16/2022, 6:20 PM

I’m working on improving the temporary

get_option

API to resolve environment-sensitive values to add: 1. static type safety 2. making it a (runtime?) error to access environment-sensitive variables directly off the subsystem

👍 1

curved-television-6568

09/16/2022, 6:57 PM

Hi :

hundreds-father-404

09/16/2022, 7:20 PM

@ancient-vegetable-10556 does this new approach mean you think global options might work with your

get_option

? I'm trying to figure out what to do with the global option

remote_execution_extra_platform_properties

. (I'll try this out rn)

ancient-vegetable-10556

09/16/2022, 7:22 PM

Yes.

ancient-vegetable-10556

09/16/2022, 7:22 PM

GlobalOptions

is a

Subsystem

, so it should Just Work™

❤️ 1

witty-crayon-22786

09/16/2022, 7:23 PM

doesn’t that mean that ~~basically~~ all consumption of environment specific options would need to migrate to a new API?

ancient-vegetable-10556

09/16/2022, 7:24 PM

Yes. The alternative is that anything that wants to consume parsed options independently of environments must do so with a new mechanism that does not presently exist in a useful form

witty-crayon-22786

09/16/2022, 7:25 PM

and the issue preventing

Subsystem

construction from consuming the environment is that field defaults are used at some point during

EnvironmentTarget

construction, which causes a cycle?

ancient-vegetable-10556

09/16/2022, 7:26 PM

No, they’re created entirely independently of

EnvironmentTarget

, and there is a Gigantic Flipping Question Mark around what it would mean for

EnvironmentTarget

to be present when they are constructed

ancient-vegetable-10556

09/16/2022, 7:27 PM

there is no cycle, there is just an absence of parameters

witty-crayon-22786

09/16/2022, 7:27 PM

…oh. then they need to be marked

@union(in_scope_types=[EnvironmentName])

…

hundreds-father-404

09/16/2022, 7:27 PM

I strongly support Chris's revised plan. It keeps subsystems a singleton, rather than making N subsystems per N environments. Those semantics are much clearer

hundreds-father-404

09/16/2022, 7:28 PM

It's fine to have slightly more verbose call sites for the 5-10% of options that are environment specific if it means everything is clearer and we avoid a bunch of semantic issues from subsystems no longer being singletons.

slightly more verbose call sites

This only impacts plugin authors, not end users

ancient-vegetable-10556

09/16/2022, 7:29 PM

@witty-crayon-22786 This is not a “fix it in code” issue absent actually modelling a new interpretation of BUILD files

👍 1

hundreds-father-404

09/16/2022, 7:29 PM

yeah, we spent some time trying to "fix" things and both came to the same conclusion that this approach is much better

witty-crayon-22786

09/16/2022, 7:30 PM

Copy code

diff --git a/src/python/pants/engine/target.py b/src/python/pants/engine/target.py
index b77b0c0ad1..cff934776f 100644
--- a/src/python/pants/engine/target.py
+++ b/src/python/pants/engine/target.py
@@ -264,7 +264,7 @@ class AsyncFieldMixin(Field):
         )
 
 
-@union
+@union(in_scope_types=[EnvironmentName])
 @dataclass(frozen=True)
 class FieldDefaultFactoryRequest:
     """Registers a dynamic default for a Field.

ancient-vegetable-10556

09/16/2022, 7:30 PM

yes, you can do that

ancient-vegetable-10556

09/16/2022, 7:30 PM

but now there’s an interpretation issue for the relevant fields

witty-crayon-22786

09/16/2022, 7:31 PM

that will be present approximately everywhere: 98% of unions are marked this way.

witty-crayon-22786

09/16/2022, 7:31 PM

i apparently forgot this one… sorry.

sparse-lifeguard-95737

09/16/2022, 7:32 PM

Testing out Pants 2.14 and hit a surprising change of behavior. I honestly don’t know how the previous behavior worked at all, but wanted to flag it in case it’s a regression. We have a dir with:

Copy code

common.sh
test_common.sh
BUILD.pants

where

test_common.sh

invokes

common.sh

, and the

BUILD.pants

file had:

Copy code

shell_sources()

shunit2_tests(name="tests", shell="bash")

this worked fine in v2.13, but on v2.14.0rc0

test_common.sh

started failing with a “no such file or directory” error when it tried to invoke

common.sh

. adding an explicit dependency in the

BUILD.pants

made things work again

witty-crayon-22786

09/16/2022, 7:34 PM

the “interpretation issue” sounds like the same issue we have discussed in the context of computing

TransitiveTargets

DependenciesRequests

, etc: yes, they will be computed multiple times unless we explicitly pin them to being computed in a particular environment

witty-crayon-22786

09/16/2022, 7:34 PM

that’s a performance + semantics decision that we will need to make at some point soonish.

witty-crayon-22786

09/16/2022, 7:35 PM

but at the moment, the

EnvironmentName

can be approximately everywhere

ancient-vegetable-10556

09/16/2022, 7:42 PM

I’m trying to figure out what it means to have a target that specifies both an environment and a field that defaults to an environment-sensitive option, and whether the explanation of that is immediately obvious

ancient-vegetable-10556

09/16/2022, 7:44 PM

it’s not a matter of “can we do it”, it’s a matter of “does it make sense”

👍 2

witty-crayon-22786

09/16/2022, 7:45 PM

Yea, that's interesting. I think that this is the difference between "marking some stuff as definitely not consuming the environment" vs "allowing everything to consume the environment, and then using consistent environments for various tasks in the graph"

ancient-vegetable-10556

09/16/2022, 7:48 PM

Exactly, and maybe the answer to my question is obvious, but right now, it feels a lot saner to me to make consuming environment-sensitive options a thing that you have to opt into explicitly — it’ll just be a matter of asking for the environment — than it does for the environment to just magically be there

👍 1