https://pantsbuild.org/ logo
Powered by
#development
  • w

    witty-crayon-22786

    09/15/2022, 9:38 PM
    this is why well-defined/user-defined order
  • h

    hundreds-father-404

    09/15/2022, 9:40 PM
    that is also in the Appendix…
    Sort of -- targetless builds aren't mentioned, which is specifically why I think we need an option to disambiguate the RE environment when n > 1
  • w

    witty-crayon-22786

    09/15/2022, 9:40 PM
    what is “targetless” here? the environment not being set?
  • w

    witty-crayon-22786

    09/15/2022, 9:41 PM
    ah, “no 
    target
    , just files”… sure.
  • h

    hundreds-father-404

    09/15/2022, 9:41 PM
    targetless is one of our implementations that literally don't consider targets: 
    count-loc
    , 
    regex-lint
    , Black when it's run on BUILD files, etc
  • w

    witty-crayon-22786

    09/15/2022, 9:41 PM
    that’s 
    __unconstrained__
    though, i assume?
  • h

    hundreds-father-404

    09/15/2022, 9:41 PM
    If you have 
    __unconstrainted__
    and 2 RE environments, which do we use?
  • w

    witty-crayon-22786

    09/15/2022, 9:42 PM
    well defined ordering
  • w

    witty-crayon-22786

    09/15/2022, 9:42 PM
    To match the 
    __unconstrained__
    environment matcher, the engine would proceed in a well/user-defined order (although unfortunately TOML tables do not maintain ordering) through the defined environments, applying the matcher to each of them, while applying any other relevant constraints from each environment. For example: a 
    local_environment
    would match its platform against the current platform, whereas 
    remote_environment
    might simply match iff remote execution is actually enabled.
  • w

    witty-crayon-22786

    09/15/2022, 9:42 PM
    sorry, implicit in that is “first that matches (in a well-defined order)”
  • h

    hundreds-father-404

    09/15/2022, 9:47 PM
    See the last paragraph about swapping from a dict/TOML-table to a list. The order of environments in that list would be the user-defined/well-defined order.
    SG I think One challenge here is if we do want to keep it that RE can work when n=0 for RE targets. I think it's actually pretty nonsensical to have n=0 though -- it's almost certain that your localhost environment settings should be different than the RE settings. Thoughts?
  • w

    witty-crayon-22786

    09/15/2022, 9:48 PM
    One challenge here is if we do want to keep it that RE can work when n=0 for RE targets.
    oh. hm… i think that it would be ok to just deprecate the global platform-properties
  • w

    witty-crayon-22786

    09/15/2022, 9:49 PM
    remote execution is an advanced enough use-case, that i’m not worried about having the same sort of fallback we do for local
    ➕ 1
  • w

    witty-crayon-22786

    09/15/2022, 9:49 PM
    so requiring a 
    remote_environment
    seems fine.
    👍 1
  • h

    hundreds-father-404

    09/15/2022, 9:51 PM
    Sg, thanks. So this idea of 
    __unconstrained__
    and a list of what the priority should be could have big implications, particularly with target-less and Docker. I think it means we now have a way to specify target-less goals like 
    count-loc
    should run via Docker? Basically, it sets the global default. And that also impacts 
    fmt
    , 
    lint
    , etc.
    Basically, it sets the global default.
    Then targets can get more granular to override the global default
  • w

    witty-crayon-22786

    09/15/2022, 9:52 PM
    yea. the implication would be that if you define both a local environment and a docker environment, but put the docker environment first in the list, it would be used by default for 
    __unconstrained__
    👍 1
  • h

    hundreds-father-404

    09/15/2022, 9:54 PM
    Basically, it sets the global default. And that also impacts fmt, lint, etc.
    Sg. So then does it make sense to add the 
    environment
    field for those goals? I don't think it's any extra work to get Docker working with 
    lint
    etc Only tricky part is this I think
    Unlike the resolve field, we do not consider the environment of dependencies. We only base the environment on the "root" target. This dramatically simplifies the user experience because you don't need to make sure your transitive closure is compatible.
    • One complication is batched runs, like fmt, lint, and check. A batch must happen in the same environment, by definition. So we will need to partition.
    So, we could start with you having to change the global default to get those goals using Docker or RE
  • a

    ancient-vegetable-10556

    09/15/2022, 9:57 PM
    @witty-crayon-22786 I’m working on the Subsystems work, at the moment I don’t understand how to get an 
    EnvironmentName
    is passed into 
    _construct_subsystem
    . Can you talk me through it?
  • w

    witty-crayon-22786

    09/15/2022, 9:57 PM
  • a

    ancient-vegetable-10556

    09/15/2022, 9:58 PM
    That is exactly what I’m looking at
  • w

    witty-crayon-22786

    09/15/2022, 9:59 PM
    currently it’s partially applied such that the final rule that is constructed takes zero positional arguments
  • w

    witty-crayon-22786

    09/15/2022, 9:59 PM
    to have it take the 
    EnvironmentTarget
    , you’d adjust 
    def construct_subsystem
    (and fix its spelling, oy)
  • w

    witty-crayon-22786

    09/15/2022, 10:00 PM
    then you’d adjust the 
    TaskRule
    to say that it takes a positional argument of the EnvironmentTarget
  • a

    ancient-vegetable-10556

    09/15/2022, 10:00 PM
    OK, let me see if I can make that work
  • a

    ancient-vegetable-10556

    09/15/2022, 10:00 PM
    EnvironmentTarget
    or 
    EnvironmentName
    ?
  • w

    witty-crayon-22786

    09/15/2022, 10:01 PM
    depends what you’re trying to do in there
  • w

    witty-crayon-22786

    09/15/2022, 10:01 PM
    the former is easier if that’s what you actually need to apply the options
  • a

    ancient-vegetable-10556

    09/15/2022, 10:01 PM
    I’d be constructing a new subsystem for every 
    EnvironmentTarget
    to apply the options
  • w

    witty-crayon-22786

    09/15/2022, 10:01 PM
    so yea, probably EnvironmentTarget
  • w

    witty-crayon-22786

    09/15/2022, 10:02 PM
    Copy code
    diff --git a/src/python/pants/option/subsystem.py b/src/python/pants/option/subsystem.py
index 9c3f6392da..c550a9e119 100644
--- a/src/python/pants/option/subsystem.py
+++ b/src/python/pants/option/subsystem.py
@@ -68,6 +68,7 @@ class Subsystem(metaclass=ABCMeta):
             output_type=cls,
             input_selectors=(),
             func=partial_construct_subsystem,
+            input_selectors=(EnvironmentTarget,),
             input_gets=(
                 AwaitableConstraints(
                     output_type=ScopedOptions, input_types=(Scope,), is_effect=False
@@ -130,6 +131,6 @@ class Subsystem(metaclass=ABCMeta):
 _SubsystemT = TypeVar("_SubsystemT", bound=Subsystem)
 
 
-async def _construct_subsytem(subsystem_typ: type[_SubsystemT]) -> _SubsystemT:
+async def _construct_subsytem(subsystem_typ: type[_SubsystemT], environment_target: EnvironmentTarget) -> _SubsystemT:
     scoped_options = await Get(ScopedOptions, Scope(str(subsystem_typ.options_scope)))
     return subsystem_typ(scoped_options.options)
1...859860861...906Latest