https://pantsbuild.org/ logo
Join Slack
Powered by
Hi, I've been trying to write a patch for system-b...
# general
m
Hi, I've been trying to write a patch for system-binaries subsystem (https://github.com/pantsbuild/pants/pull/19377) that I hope someone can look at and let me know if it's the right kind of idea for the issues (#14492, #19375). It doesn't implement functionality for per-binary paths but it does allow you to prepend or replace the default search path 
("/usr/bin", "/bin", "/usr/local/bin", "/opt/homebrew/bin")
.
👋 1
h
Thanks for tackling this! I think the per-binary paths functionality may be overkill, so you're right not to implement that IMO.
I just speed-read through your draft PR and it seems directionally correct. I'd suggest that instead of 
prepend_path
we support a special symbol like 
<PATH>
(with literal angle brackets) to represent the contents of $PATH. That would let you embed it anywhere rather than only prepending. it would also reduce naming confusion, because "path" (lower-case) means all sorts of things in this context... Plus we already use this convention in several places (see 
git grep "<pants>"
m
I initially attempted that with a combination of 
EnvironmentAware(ExecutableSearchPathsOptionMixin) # etc
and a BoolOption that when set would use PATH from the environment, but I couldn't get that PATH in the 
system_binary_paths
property because (I'm not 100% sure if this is correct) the subsystem isn't instantiated yet. If you know where there's an example or could lead me down the correct path I do think that's an even better solution than either of mine
h
You shouldn't need a 
BoolOption
AFAICT. Instead of you see that literal string in the user's value for the path option, you replace it with the contents of 
os.environ["PATH"]
c
m
I've pushed that change - not sure how you'd want a test for this to look like but can work from an example if you know where to find a subsystem test for this-kind-of-thing
Open in Slack
PreviousNext
Powered by