Factor out lookupExecutable and other PATH improvements#11218
Factor out lookupExecutable and other PATH improvements#11218Ericson2314 merged 2 commits intoNixOS:masterfrom
lookupExecutable and other PATH improvements#11218Conversation
github-actions
bot
added
new-cli
Ericson2314
force-pushed
the
better-executable-path
branch
from
57c9d8a to
3c761de
Compare
Ericson2314
changed the title
lookupExecutable and other PATH improvments
Ericson2314
force-pushed
the
better-executable-path
branch
from
3c761de to
5e6e965
Compare
| #ifdef WIN32 | ||
| # define PATH_VAR_SEP ";" | ||
| #else | ||
| # define PATH_VAR_SEP ":" | ||
| #endif |
There was a problem hiding this comment.
Do we need to do this again, when executable-path.cc already has the (weirdly capitalized) pATH_separator?
There was a problem hiding this comment.
I did something similar for unit tests before, I don't mind having the unit tests have their own redundant one-off definition just for a bit of spec vs implementation separation purposes.
src/libutil/executable-path.cc
Outdated
|
|
||
| std::vector<fs::path> parsePATH(const OsString & path) | ||
| { | ||
| auto strings = basicTokenizeString<std::list<OsString>, OsString::value_type>(path, pATH_separator); |
There was a problem hiding this comment.
I punted on the path lookup case (with an assert, documented of course) (as opposed to name look up case) since I am not sure sort of error message we want to do, and as it turns out in most cases we are just looking up statically known names.
This brings us back into compliance. I think just in the next PR, for the build hook, we look up a dynamic choice. So we can see what we want to do then.
Ericson2314
force-pushed
the
better-executable-path
branch
from
5e6e965 to
7c6c854
Compare
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: |
Ericson2314
force-pushed
the
better-executable-path
branch
2 times, most recently
from
5f8393b to
a6d8ba7
Compare
roberth
changed the title
lookupExecutable and other PATH improvmentslookupExecutable and other PATH improvements
|
#11178 (comment) Naming question about "OS" vs "Native" that needs to be resolved. |
Ericson2314
force-pushed
the
better-executable-path
branch
2 times, most recently
from
4e693f3 to
1d55514
Compare
Ericson2314
force-pushed
the
better-executable-path
branch
from
1d55514 to
1216778
Compare
This ended up motivating a good deal of other infra improvements in order to get Windows right: - `OsString` to complement `std::filesystem::path` - env var code for working with the underlying `OsString`s - Rename `PATHNG_LITERAL` to `OS_STR` - `NativePathTrait` renamed to `OsPathTrait`, given a character template parameter until NixOS#9205 is complete. Split `tests.cc` matching split of `util.{cc,hh}` last year. Co-authored-by: Robert Hensing <[email protected]>
Ericson2314
force-pushed
the
better-executable-path
branch
from
20acf6b to
6c861b9
Compare
roberth
left a comment
roberth
left a comment
There was a problem hiding this comment.
Clarification and optional suggestion, but other than that, LGTM
| auto s2 = v.render(); | ||
| EXPECT_EQ(s2, OS_STR("." PATH_VAR_SEP "." PATH_VAR_SEP "." PATH_VAR_SEP ".")); | ||
| } | ||
|
|
There was a problem hiding this comment.
It'd be nice to use the isExecutableFile parameter to add a (pure) unit test here.
The test could use a lambda that returns true for a fixed set of paths.
| // "If the pathname being sought contains a <slash>, the search | ||
| // through the path prefixes shall not be performed." | ||
| // https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap08.html#tag_08_03 | ||
| assert(OsPathTrait<fs::path::value_type>::rfindPathSep(exe) == exe.npos); |
There was a problem hiding this comment.
I think they mean to just return exe so that it's later resolved relative to the working directory.
There was a problem hiding this comment.
I am pretty sure shells do not let you run things in the current directory without ./ as a safety feature? But yeah that is probably extra effort on their part. Regardless, the other find function in the next commit will always return something.
There was a problem hiding this comment.
That is correct, but it doesn't mean that a command with a slash is wrong.
$ (export PATH=/foo; cd result/bin; hello ; )
hello: command not found
$ (export PATH=/foo; result/bin/hello ; )
Hello, world!other find function in the next commit
Doesn't seem to be part of this PR.
I don't think this assertion is useful. The lack of a directory separator is easily observed in the string literals that we tend to pass to this function.
Co-authored-by: Robert Hensing <[email protected]>
6 participants
Motivation
This ended up motivating a good deal of other infra improvements in order to get Windows right.
Contains some other mics Windows/Meson fixes because there were regressions.
Context
Good for Windows support in general, also buttresses #11178 slightly.
Priorities and Process
Add 👍 to pull requests you find important.
The Nix maintainer team uses a GitHub project board to schedule and track reviews.