-
-
Notifications
You must be signed in to change notification settings - Fork 15.3k
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
semver-tool: init at 2.1.0 #51004
semver-tool: init at 2.1.0 #51004
Conversation
Failure on x86_64-linux (full log) Attempted: semver-tool Partial log (click to expand)
|
Failure on aarch64-linux (full log) Attempted: semver-tool Partial log (click to expand)
|
I chose "semver-tool" as the path for this package because "semver" is extremely generic, and probably more rightly belongs to node-semver[1] (at least judging by GitHub stars), and because "semver-tool" is the name of this project's GitHub repository, and because it was the name used in the package request[2]. Closes NixOS#50945. [1]: https://github.com/npm/node-semver [2]: NixOS#50945
Failure on x86_64-linux (full log) Attempted: semver-tool Partial log (click to expand)
|
Failure on aarch64-linux (full log) Attempted: semver-tool Partial log (click to expand)
|
Probably need to have |
The user of this package won't necessarily want to install to /usr/local (indeed, that directory doesn't even exist on my system). Additionally, package managers would probably like to install the program elsewhere[1], and without a way to customize the prefix would have to fall back to either patching the Makefile, or doing the installation manually, both of which might break down the line. By introducing a PREFIX variable with a default value of /usr/local, the installation prefix can be customized, solving these problems. [1]: Example: NixOS/nixpkgs#51004
Hardcoded prefix reported upstream: fsaintjacques/semver-tool#24 |
}; | ||
|
||
postPatch = '' | ||
patchShebangs . |
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.
You should only patch the tests – this will patch src/semver
with build-time bash 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.
Ah, didn't occur to me that build-time bash could be different from runtime. But of course they can (eg if crosscompiling, right?).
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.
But then the tests will want to run src/semver
unpatched, and that won't work, right?
Success on aarch64-linux (full log) Attempted: semver-tool Partial log (click to expand)
|
Success on x86_64-linux (full log) Attempted: semver-tool Partial log (click to expand)
|
Motivation for this change
Requested by @coretemp in #50945.
I chose "semver-tool" as the path for this package because "semver" is extremely generic, and probably more rightly belongs to node-semver (at least judging by GitHub stars), and because "semver-tool" is the name of this project's GitHub repository, and because it was the name used in the package request.
Things done
sandbox
innix.conf
on non-NixOS)nix-shell -p nox --run "nox-review wip"
./result/bin/
)nix path-info -S
before and after)