Add fallback_environment for remote_environment
#16955
Conversation
[ci skip-rust] [ci skip-build-wheels]
Eric-Arellano
force-pushed
the
remote-fallback
branch
from
99d5a16 to
0bb483f
Compare
Eric-Arellano
changed the title
fallback_environment for remote_environmentfallback_environment for remote_environment
Eric-Arellano
added
the
category:internal
| if ( | ||
| not tgt.has_field(CompatiblePlatformsField) | ||
| and not tgt.has_field(DockerImageField) | ||
| and not tgt.has_field(RemotePlatformField) |
There was a problem hiding this comment.
Oops. This blocked RE targets actually being usable
[ci skip-rust] [ci skip-build-wheels]
|
|
||
| Must be an environment name from the option `[environments-preview].names`, the | ||
| special string `{LOCAL_ENVIRONMENT_MATCHER}` to use the relevant local environment, or the | ||
| string `{_NO_FALLBACK_ENVIRONMENT}` to error when remote execution is disabled. |
There was a problem hiding this comment.
I think that falling back by default to the local environment is error prone, so I think that this is potentially inverted.
The chances of a local environment being compatible with the remote environment without extra consideration/attention paid to setting up all of the env vars correctly is pretty slim: and it seems better to fail fast in that case ("you probably haven't made this compatible yet"), rather than to fail slowly ("I fell back automatically for you, and now something is failing in an unexpected way mid build").
Having this be None (the literal value) by default, and failing if remote execution cannot be used would be preferable I think, and avoids a special case.
# 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]
| @@ -204,12 +204,36 @@ class RemoteExtraPlatformPropertiesField(StringSequenceField): | |||
| ) | |||
|
|
|||
|
|
|||
| _NO_FALLBACK_ENVIRONMENT = "<none>" | |||
There was a problem hiding this comment.
With None as the default, this value is no longer necessary.
# 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]
…nvironment` (#16971) Follow up to #16955. It makes sense to also allow fallbacks for local environments and docker environments, because using those environments is fallible: - local environment might not match the platform - docker environment might not match the platform when using Linux because it does not use virtualization, unlike macOS, to handle a different CPU arch The user may want to do things like prefer local if on Linux, but fall back to Docker if on macOS. [ci skip-rust] [ci skip-build-wheels]
Closes #16907.
This feature is particularly powerful when you are using a Docker image for remote execution and you set the fallback environment is a Docker environment: it means you can always use the same container.
Note that we considered instead having consuming targets set this fallback logic in the
environment: strfield. This would mean adding adocker_imagefield to theremote_executionenvironment target where the user gives us a hint what the value is, and then call sites use a new DSL likeenvironment="image:centos6. This PR was accepted in https://pantsbuild.slack.com/archives/C0D7TNJHL/p1663790578352079 as a simper design, especially because we want to keep things simpler for consumers of this feature, which make up the majority of the users.