https://pantsbuild.org/ logo
Join Slack
Powered by
draft blog post for the in-workspace execution sup...
# development
f
draft blog post for the in-workspace execution support: https://docs.google.com/document/d/1TlBsbIdghj-MGl0KyvNSPlJiXk6n8FkUcE9hzg8zXfk/edit?usp=sharing comments welcome!
p
Nice. I found workspace environments to be the best way to capture files from 
.git/modules
directory (an admittedly questionable thing to do). One quirk I've noticed is that I get a warning when I run 
pants lint ::
about linting my 
shell_command
target, but why would that target need to be linted? Does it run 
shellcheck
on the 
command
? If so, we might want 2 environment fields, one for running the command and another for linting it. Because linting does not need to run in the workspace, and I would rather it didn't. Oh, I could probably skip lint on that target. You might want to add a limitation note about how the environment will be used for all goals including running, pointing, etc.
f
You might want to add a limitation note about how the environment will be used for all goals including running, linting, etc.
I was not aware linting ran on 
shell_command
targets. The better solution would seem to be your suggestion of two targets: one for "execution" and one for (for lack of a batter term) "non-execution"
Do you have a 
shell_sources
in the same directory as the 
shell_command
target? Maybe you have a shell script being picked up that way? (Looking at the shellcheck rules, it registers only for 
shell_source
and 
shunit2_test
target types.)
There is a 
ShellCommandSourcesField
private field called 
_sources
which I wonder if it has some relevance or not.
p
Yes, there is a 
shell_sources
target in the same BUILD file, but when I run 
pants lint ::
the warning specifically mentions the `shell_command`'s address:
Copy code
20:38:50.07 [WARN] The lint goal was called with target `//:capture_git_modules`, which specifies the environment `in_repo_workspace`, which is a `experimental_workspace_environment`. The lint goal only runs in the local environment. You may experience unexpected behavior.
You can see that target here: https://github.com/StackStorm/st2/blob/master/BUILD#L105-L120 And the environment definition here: https://github.com/StackStorm/st2/blob/master/BUILD.environment#L10-L23 Oh. The visibility backend lints all targets. I guessed wrong, shellcheck is not linting the 
shell_command
.
Copy code
$ pants lint //:capture_git_modules
20:48:02.55 [WARN] The lint goal was called with target `//:capture_git_modules`, which specifies the environment `in_repo_workspace`, which is a `experimental_workspace_environment`. The lint goal only runs in the local environment. You may experience unexpected behavior.
20:48:02.79 [INFO] Completed: Check for visibility rule violations - visibility succeeded.

✓ visibility succeeded.
The visibility backend doesn't "run" anything however, so that warning is useless. Maybe we need a way for the backends to opt out of that warning? https://github.com/pantsbuild/pants/blob/main/src/python/pants/backend/visibility/lint.py#L39
f
EnvironmentTarget
has some helpers for rules to use to know when a target is “local”.
p
So, maybe you need a note that a known limitation is spurious warnings when using the visibility lint backend? This is where the warning gets raised: https://github.com/pantsbuild/pants/blob/main/src/python/pants/core/goals/lint.py#L367 https://github.com/pantsbuild/pants/blob/main/src/python/pants/core/util_rules/environments.py#L449 Is the in workspace environment not considered local?
f
Maybe the visibility rules should use that to determine when the environment is “local”?
For example 
can_use_system_path_metadata_requests
p
Looks like it's using an 
isinstance
check to determine if its local: 
isinstance(env_tgt.val, LocalEnvironmentTarget)
https://github.com/pantsbuild/pants/blob/main/src/python/pants/core/util_rules/environments.py#L485
f
But not that one in particular.
That code should stop using an 
isinstance
check.
p
Looks like there are several places in the codebase that use a similar isinstance check. They might need to be replaced with a property on 
EnvironmentTarget
that works just like 
can_use_system_path_metadata_requests
, maybe called 
is_local_compatible
?
Copy code
$ git grep 'isinstance([^,]*, LocalEnvironmentTarget)'
src/python/pants/core/util_rules/adhoc_binaries.py:    if env_tgt.val is None or isinstance(env_tgt.val, LocalEnvironmentTarget):
src/python/pants/core/util_rules/asdf.py:    if not (isinstance(env_tgt.val, LocalEnvironmentTarget) or env_tgt.val is None):
src/python/pants/core/util_rules/environments.py:        if env_tgt.val is not None and not isinstance(env_tgt.val, LocalEnvironmentTarget)
src/python/pants/core/util_rules/search_paths.py:    if not (request.env_tgt.val is None or isinstance(request.env_tgt.val, LocalEnvironmentTarget)):
src/python/pants/core/util_rules/search_paths.py:    if env is None or isinstance(env, LocalEnvironmentTarget):
Those util rules are actually not following the pants convention of identifying a target by its fields. 😛
f
• 
src/python/pants/core/util_rules/adhoc_binaries.py:52
-- Checks whether a Python executable is available "locally" to the rule code (so available directly and not, for example, running in a Docker container) • 
src/python/pants/core/util_rules/environments.py:485
-- 
_warn_on_non_local_environments
function which checks whether the environment is "non-local". It is a helper used in other parts of the code to check for non-local environments. (Visibility rules should probably use it.) • 
src/python/pants/core/util_rules/search_paths.py:54
-- Checks to see if paths for a "versions manager" like pyenv are available locally. • 
src/python/pants/core/util_rules/search_paths.py:124
-- Similar check whether discovered paths can only be discovered locally. • 
src/python/pants/core/util_rules/asdf.py:100
-- Another check whether ASDF-discovered paths can be directly accessed in the current environment. (If not available locally, the ASDF provider just reports no paths.)
The common element to me seems to me to be "supports accessing paths in environment directly (without invoking a Process)"
p
The visibility backend is a lint backend. It does not directly call 
_warn_on_non_local_environment
, but that method gets called for all lint backends in the base code.
f
The visibility backend is a lint backend. It does not directly call 
_warn_on_non_local_environment
, but that method gets called for all lint backends in the base code.
Ah so the warning you saw probably originated there?
👍 1
p
Oh. I see the logic of 
can_use_system_path_metadata_requests
name. I assumed that 
SystemPathMetadataRequest
was a specific dataclass request as input for some specific rule.
f
yeah 
can_use_system_path_metadata_requests
just means that the 
PathNamespace.SYSTEM
can be used with 
PathMetadataRequest
p
So, we don't need another method then. That one does just what all those callsites need 😛, and the name is correct too (even if I didn't understand it at first, it makes sense now).
f
well I'm not wedded to the name, can rename to make it less confusing and less specific to 
PathMetadataRequest
p
Is 
can_use_system_paths
overly generic?
f
supports_direct_client_access_to_files_in_environment
Seems like there is a concept of the environment being in the same place as where the Pants client is running.
p
can_use_client_system_paths
? 
can_use_pants_client_system_paths
?
f
yeah & much more succinct than my idea
p
Or reuse the term local that we use everywhere else: 
can_use_local_system_paths
Naming things is hard ™️
f
sticking with "local" in the name like your last suggestion seems fine to me.
and I agree, naming is hard!
Open in Slack
PreviousNext
Powered by