Pants 2.24 - "IntrinsicError: path_metadata_request error: path for PathNamespace.SYSTEM must an absolute path. Instead, got ``"

[darrenclark]

Describe the bug
On Pants 2.24 I started getting an error when running pants generate-lockfiles --resolve=nodejs-default:

$ pants generate-lockfiles --resolve=nodejs-default
10:53:49.91 [ERROR] 1 Exception encountered:

Engine traceback:
  in `generate-lockfiles` goal

IntrinsicError: path_metadata_request error: path for PathNamespace.SYSTEM must an absolute path. Instead, got ``

Pants version
2.24.0

OS
MacOS

Additional info

I tracked down the issue to an empty path (::) in my PATH, ie:

export PATH=/path/one:/path/two::/path/three

It'd be nice if the error message included something like "please check your PATH for any empty entries (::)"

[sureshjoshi]

I vaguely remember some discussion in the past year or two, about whether PATHs should be sanitized on print, printed out at all, could they leak data, etc etc etc.

An aside from all that, depending on where this is called from and whether we consume the whole path, or if we iterate through the PATH entries, we might even be able to point to the char index in the PATH (if we didn’t want to print out the PATH itself).

[tdyas]

It is also trivial to just silently skip or warn about empty path entries in the relevant logic:

pants/src/python/pants/core/util_rules/system_binaries.py

Lines 655 to 658 in 684413b

	metadata_results = await MultiGet(
	Get(PathMetadataResult, PathMetadataRequest(path=path, namespace=PathNamespace.SYSTEM))
	for path in request.search_path
	)

We should also probably add a description_of_origin field to BinaryPathRequest so that the warning can annotate where the bad path originated. But that is a more intrusive change.

[tdyas]

And that description_of_origin could be passed into the intrinsic rule so the origin properly displays in the error. (I don't know if the engine allow rules to catch engine-thrown exceptions and re-raise them.)

[benjyw]

Yeah, just ignoring empty path entries seems fine? it has no meaning AFAICT.

@darrenclark want to take a crack at it? It may be as simple as changing that to for path in request.search_path if path, but you're best positioned to test it.

[cburroughs]

@darrenclark thank you for the report, would you mind checking if this still occurs with 2.24.2rc0 which includes #21916 ?

[jsirois]

Yeah, just ignoring empty path entries seems fine? it has no meaning AFAICT.

Not in bash:

:; cat << EOF > hey-what-about-me?
∙ #!/usr/bin/env bash
∙ 
∙ echo "I'm Alive!"
∙ EOF

:; chmod +x hey-what-about-me\? 

:; ./hey-what-about-me\? 
I'm Alive!

:; hey-what-about-me\? 
hey-what-about-me?: command not found

:; PATH=":$PATH" hey-what-about-me\? 
I'm Alive!

:; PATH="::$PATH" hey-what-about-me\? 
I'm Alive!

:; PATH="$PATH:" hey-what-about-me\? 
I'm Alive!

:; PATH="$PATH::" hey-what-about-me\? 
I'm Alive!

I don't use this sort of setup, but I've known many people who do. They always want CWD on the PATH. More typically they add . to the path, but empty (``) means the same thing as demonstrated above.

[benjyw]

Ah, if empty means the same as . then yeah, we should handle it properly.

[tdyas]

To clarify, these are paths being used for searching for binaries for system_binaries. Should empty or . just mean the build root in that case?

[benjyw]

They do (and should) mean the CWD. Which most of the time is the build root, but doesn't strictly have to be.

[darrenclark]

No luck with 2.24.2rc2:

[darrenclark]

@darrenclark want to take a crack at it? It may be as simple as changing that to for path in request.search_path if path, but you're best positioned to test it.

Ah, if empty means the same as . then yeah, we should handle it properly.

They do (and should) mean the CWD. Which most of the time is the build root, but doesn't strictly have to be.

Sure! I will take a go at this

[tdyas]

They do (and should) mean the CWD. Which most of the time is the build root, but doesn't strictly have to be.

Right, but to clarify my point: There are multiple processes in play here so the question becomes: Which process's working directory should be chosen when PATH is incorporated into --system-binaries-system-binary-paths via the <PATH> marker? For example, should we choose the Pants client (which could be run from the build root or a subdirectory) or the pants daemon (probably build root)?

Allowing . as current working directory does make sense for the shell since then PATH is used in a single process which has its own working directory. Current working directory makes less sense in my view for --system-binaries-system-binary-paths since working directory is ambiguous there unless we define which process is relevant.

If we choose the pants daemon, for example, then we should just resolve blank strings or . to the build root and give the intrinsic the actual absolute path. And that should happen at the code sites for the --system-binaries-system-binary-paths option.

[darrenclark]

I pushed up a commit here that seems to fix things for me - main...darrenclark:fix-issue-21954

As a side effect, it seems like ~ gets evaluated in the PATH now - not sure if that is desirable or not

[richardgirges]

I'm seeing this exact same error while running in the pants export venv:

pants export --py-resolve-format=symlinked_immutable_virtualenv --resolve=python-default
source dist/export/python/virtualenvs/python-default/3.11.6/bin/activate
pants package models/text_classifier_service

output:

11:31:57.33 [ERROR] 1 Exception encountered:

Engine traceback:
  in `package` goal

IntrinsicError: path_metadata_request error: path for PathNamespace.SYSTEM must be an absolute path. Instead, got `$PATH`

Seeing it on the latest stable 2.24 and the latest RC that was released ~13 hrs ago. I don't see any empty entries in $PATH though, so I'm not sure what's causing it. The error only happens when I'm running pants package - other commands seem to work fine.

I have to work around it by spinning up a non-virtualenv shell for now.

[benjyw]

I pushed up a commit here that seems to fix things for me - main...darrenclark:fix-issue-21954

As a side effect, it seems like ~ gets evaluated in the PATH now - not sure if that is desirable or not

Thanks for posting that patch. FWIW, I think that a simple unconditional abspath() will cover these cases:

$ pwd
/Users/benjyw/src/pants
$ python 
Python 3.11.9 (main, Aug 30 2024, 14:22:39) [Clang 15.0.0 (clang-1500.3.9.4)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import os
>>> os.path.abspath("")
'/Users/benjyw/src/pants'
>>> os.path.abspath(".")
'/Users/benjyw/src/pants'
>>> os.path.abspath("rel/path")
'/Users/benjyw/src/pants/rel/path'
>>> os.path.abspath("/abs/path")
'/abs/path'
>>> 

And also, I think os.path.expanduser() is needed to expand ~, so I'm not sure what you mean by it getting evaluated in the PATH?