-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve command not found errors #2498
Conversation
src/libexpr/primops/fetchGit.cc
Outdated
std::ostringstream out; | ||
out << e.what() << std::endl; | ||
out << "The program 'git' is currently not installed. It is required by builtins.fetchGit" << std::endl; | ||
out << "You can install it by typing one of the following:" << std::endl; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
out << "You can install it by typing one of the following:" << std::endl; | |
out << "You can install it by running the following:" << std::endl; |
std::ostringstream out; | ||
out << e.what() << std::endl; | ||
out << "The program 'hg' is currently not installed. It is required by builtins.fetchMercurial" << std::endl; | ||
out << "You can install it by typing one of the following:" << std::endl; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
out << "You can install it by typing one of the following:" << std::endl; | |
out << "You can install it by running the following:" << std::endl; |
This is awesome! |
218ef8b
to
4980230
Compare
src/libexpr/primops/fetchGit.cc
Outdated
if (state.allowedPaths) | ||
state.allowedPaths->insert(gitInfo.storePath); | ||
} catch (ExecError & e) { | ||
if (e.status / 256 == 127) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be WEXITSTATUS(e.status) == 127
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, didn't realise where to look for that.
src/libexpr/primops/fetchGit.cc
Outdated
out << "The program 'git' is currently not installed. It is required for builtins.fetchGit" << std::endl; | ||
out << "You can install it by running the following:" << std::endl; | ||
out << "nix-env -f '<nixpkgs>' -iA git"; | ||
throw ExecError(e.status, out.str()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should use format strings:
throw ExecError("%s\nThe program 'git' is currently not installed. ...", e.what());
ping |
Bash uses the same exit code to indictate a binary was not found in PATH.
a79f726
to
b022d8e
Compare
b022d8e
to
75c6c42
Compare
Just built the rebased version, and checked out the code at https://github.com/NixOS/nix/pull/2498/files?w=1 Looks good to me I think |
src/libexpr/primops/fetchGit.cc
Outdated
|
||
if (state.allowedPaths) | ||
state.allowedPaths->insert(state.store->toRealPath(gitInfo.storePath)); | ||
} catch (ExecError & e) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's better to put this check in exportGit
because (on the flakes branch) it's used in other places as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, should I do the same for mercurial?
Sure, should I do the same for mercurial?
Yeah, probably should :)
|
419e211
to
cfe3cbb
Compare
Still relevant but needs a rebase. Current output with flakes is:
|
I'm happy to rebase this, but it's been open for quite some time without getting attention. |
I marked this as stale due to inactivity. → More info |
I closed this issue due to inactivity. → More info |
Improves error message when git/mercurial are not available. eg.