Don't require a local_environment target for each platform
#16983
Conversation
# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
Eric-Arellano
added
the
category:internal
|
@stuhood follow up from our discussion yesterday about |
There was a problem hiding this comment.
Hm... I'm lightly negative on this.
While:
- a special case for "no environments are defined"
- using the global/option values when an environment doesn't override a field
...definitely make sense, it doesn't seem worthwhile from a consistency perspective to allow environments to be partially defined. Maybe your thinking is that it might be hard to define environments which cover all possibilities, and so the more specific (in terms of matching: centos5, etc) you need to be for some local environments, the more likely you are to have ambiguity?
There was a problem hiding this comment.
I think this is a generally good idea, and I don't have any comments on the implementation.
The use case that @Eric-Arellano highlights — needing to make a very small number of overall changes to support different Mac OS architectures — is something that I'd use immediately if we introduced it into Pants mainline and would simplify my dev environment, but I don't think we want to get to the point where we're introducing well-defined environments for every other platform we support in Pants. I think adopting environments in their entirety would be overkill for most users in this situation. We should let users adopt incrementally here.
The subsystem redesign allows users to adopt as much or as little of the target environments support as they need, and I don't think there's a good reason to arbitrarily stop people from using just a tiny bit of the functionality.
Agreed with Chris. "Consistency" here feels like "consistency for the sake of consistency", rather than optimizing for the very common and real case of: your options work for all your developers well, minus M1 devs. I don't think there's a very compelling reason to make handling that more complex than necessary; there's no technical/implementation motivation, it's actually easier to remove this restriction. |
@stuhood sounds like the concern is "light". Chris and I both feel strongly this is a good idea - is it okay to merge? |
|
Yea, fine with me. |
There's no need for us to set up local environments, now that local environments are only to _override_ values from the subsystems. (Before, we were going to deprecate the subsystems.) We may in the future want to add a macos_arm64 environment. That is safe for us to do without adding other local environments thanks to #16983. See #17043 for an alternative way we could be leveraging environments locally. [ci skip-rust]
… at all (#17135) It's now possible to use environments generally, but for the current env target to be set to None, thanks to #16983. This was resulting in faulty logic that we were treating the `--remote-execution` flag as a global switch for remoting, when really if environment targets are used at all, then we should only use remoting when a `remote_environment` target is active. [ci skip-rust] [ci skip-build-wheels]
We now are using subsystems as the default value of env vars & executable search paths. That is,
local_environmenttargets are solely needed if you want to override those option values.We want it to be ergonomic to add a
local_environmenttarget to handle one specific platform, e.g. macOS ARM often being problematic, without you needing to also add boilerplate for the other platforms. There was no good reason we were erroring for other platforms, given the new subsystem approach.[ci skip-rust]
[ci skip-build-wheels]