feat: implement command -p

[39555]

Implements the missing command -p flag to use a default value of $PATH that is guaranteed to find all of the standard utilities.

• Uses nix::libc::confstr with _CS_PATH (see confstr.3) similar to what Bash do.
• If no value is returned falls back to the STANDARD_UTILS_PATH, which is taken from Bash.

Doesn't make sense on Windows though.

[github-actions]

Performance Benchmark Report

Benchmark name	Baseline (μs)	Test/PR (μs)	Delta (μs)	Delta %
expand_one_string	3.34 μs	3.33 μs	-0.00 μs	⚪ Unchanged
instantiate_shell	60.43 μs	59.99 μs	-0.44 μs	⚪ Unchanged
instantiate_shell_with_init_scripts	29051.13 μs	29026.48 μs	-24.66 μs	⚪ Unchanged
parse_bash_completion	2739.45 μs	2742.72 μs	3.27 μs	⚪ Unchanged
parse_sample_script	4.13 μs	4.01 μs	-0.12 μs	⚪ Unchanged
run_echo_builtin_command	89.73 μs	90.29 μs	0.55 μs	⚪ Unchanged
run_one_builtin_command	108.86 μs	108.48 μs	-0.38 μs	⚪ Unchanged
run_one_external_command	1954.65 μs	1976.89 μs	22.25 μs	⚪ Unchanged
run_one_external_command_directly	1013.41 μs	1014.57 μs	1.17 μs	⚪ Unchanged

Code Coverage Report: Only Changed Files listed

Package	Base Coverage	New Coverage	Difference
brush-core/src/builtins/command.rs	🟢 91.67%	🟠 62.1%	🔴 -29.57%
brush-core/src/jobs.rs	🔴 42.42%	🔴 37.23%	🔴 -5.19%
brush-core/src/shell.rs	🟢 81.68%	🟢 81.98%	🟢 0.3%
Overall Coverage	🟢 75.12%	🟢 74.83%	🔴 -0.29%

Minimum allowed coverage is 70%, this run produced 74.83%

[reubeno]

Thanks for working on this. A few higher-level comments...

We're not quite there today with the existing code, but I'm trying to keep platform-switched logic under brush-core/src/sys, as a sort of adaptation layer. There's already existing modules there around process management, signals, terminal configuration, etc. It seems reasonable to add a helper under fs there or under a new paths module that exposes the "default" search paths for executables on the platform.

For Windows, there's a set of paths reasonable to hard-code as fallbacks (e.g., %SystemRoot%\system32, %SystemRoot%, etc.) but I think it would be okay leaving that stubbed out for now and sorted out in a separate set of changes.

For Unix-like platforms, I didn't know about confstr(). Do you know if, in practice, it ever returns anything other than the usual dir paths (e.g., /bin, /usr/bin)? If it doesn't, then I'd prefer that we avoid the unsafe code calling into it and keep the fallback set of paths you added anyway. (In general, we're trying to limit the unsafe code in this crate; we acknowledge that nix and libc have a fair bit of it, but ideally trying to leverage those crates and stay at a higher level.)

Once we sort out the above, would also be great if you could add a compat test to cover command -p. Thanks!

[39555]

Do you know if, in practice, it ever returns anything other than the usual dir paths (e.g., /bin, /usr/bin)?

Nixos returns /run/current-system/sw/bin:/bin:/usr/bin NixOS/nixpkgs#586.
Checked with the shell command getconf CS_PATH

glibc defines it as /bin:/usr/bin for unix and '' for other platforms https://github.com/bminor/glibc/blob/83a1cc3bc3d28c97d1af6c0957b11fe39fd786d8/sysdeps/unix/confstr.h.
bsd's and macos return /usr/bin:/bin:/usr/sbin:/sbin https://github.com/freebsd/freebsd-src/blob/1a37caeb076b9d31e13c54691d7f1eeb589798bb/include/paths.h#L43

[reubeno]

@39555 -- that's helpful context, thank you for digging that up. I'd love to see confstr get properly wrapped in a higher-level Unix crate like nix, which we typically utilize to provide a higher-level API on top of libc-exported bindings. (It would be interesting to know how open they'd be to taking the wrapper you've added here, or some version of it.)

With that said, I'm supportive of keeping the unsafe code in brush for now (in a reasonable place under sys, like described above), but will want to take more time to carefully review the logic and the docs for confstr().

[39555]

It is not the most urgent feature, so I think we should wait for rust-lang/libc#3612 which makes confstr available on all unix systems. Than I will make PR to the nix library.

[reubeno]

It is not the most urgent feature, so I think we should wait for rust-lang/libc#3612 which makes confstr available on all unix systems. Than I will make PR to the nix library.

That sounds like a great plan; thanks for finding that upstream PR and being willing to put in the effort to PR to nix. That will benefit other nix consumers.

[reubeno]

I'm taking a look back at this and see that the libc change was merged toward the end of last calendar year.

@39555 -- are you okay with me rebasing your change and shepherding it into the repo? Or would you like the opportunity to do so yourself? This issue has come up with recent issues we've encountered running our compat test suite on nixOS. While this change wouldn't resolve that directly, we should be able to leverage it to do so.

Edit: I've gone ahead and pushed a separate non-draft PR, rebasing your changes and making some test additions. I hope you're okay with this; will do my best to make sure you get the right credit for doing all the heavy lifting!

[reubeno]

Thanks for your work on this! Closing this now that the changes have been integrated into #402 and merged into main.