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
btrbk: Fix shebangs #97465
btrbk: Fix shebangs #97465
Conversation
@GrahamcOfBorg build btrbk |
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.
- Diff mostly LGTM
- Commits LGTM
- Add meta.changelog
- Update license to
gpl3Only
https://discourse.nixos.org/t/lib-licenses-gpl3-co-are-now-deprecated/8206 - Remove atypical/(?useless?)
inherit version
inmeta
- Builds cleanly via
nix-review
:
https://github.com/NixOS/nixpkgs/pull/97465
1 package built:
btrbk
pkgs/tools/backup/btrbk/default.nix
Outdated
@@ -12,7 +12,7 @@ stdenv.mkDerivation rec { | |||
|
|||
nativeBuildInputs = [ asciidoc asciidoctor makeWrapper ]; | |||
|
|||
buildInputs = with perlPackages; [ perl DateCalc ]; | |||
buildInputs = with perlPackages; [ perl DateCalc bash ]; |
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.
buildInputs = with perlPackages; [ perl DateCalc bash ]; | |
buildInputs = [ perlPackages.perl perlPackages.DateCalc bash ]; |
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.
Went for option number 3
If you made changes, I'm not seeing them updated on the web. |
I made some more improvements to the package. |
b475585
to
589b1af
Compare
@GrahamcOfBorg build btrbk |
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.
- Diff LGTM
- Commits LGTM
- Builds via
nix-review
https://github.com/NixOS/nixpkgs/pull/97465
1 package built:
btrbk
''; | ||
|
||
preFixup = '' | ||
wrapProgram $out/sbin/btrbk \ | ||
--set PERL5LIB $PERL5LIB \ | ||
--prefix PATH ':' "${stdenv.lib.makeBinPath [ btrfs-progs bash mbuffer openssh ]}" | ||
substituteInPlace $out/sbin/btrbk --replace "${buildPackages.bash}" "${bash}" |
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.
Why do we have to perform this substitution? i.e., why did the file in $out
end up with buildPackages.bash
?
Is it because now bash is in buildDeps
?
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.
The issue is that cross-building will cause wrapProgram
to run on the build host and the @shell@
placeholder with ${buildPackages.bash}
instead of ${pkgs.bash}
which is imo. a bug in wrapProgram
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.
I see. Yeah this is not very pretty, but I guess we can live with it.
Thanks for the PR! In general LGTM, but I have a couple of questions. |
@kampka please fix the merge conflict. |
@SuperSandro2000 PR is fine and can be merged as is. |
|
||
buildInputs = with perlPackages; [ perl DateCalc ]; | ||
buildInputs = [ bash ] ++ (with perlPackages; [ perl DateCalc ]); |
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.
buildInputs = [ bash ] ++ (with perlPackages; [ perl DateCalc ]); | |
buildInputs = (with perlPackages; [ perl DateCalc ]); |
I don't think this is needed because we do not compile against it.
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.
Ping
No, it is not. GitHub complains about a merge conflict. |
Either pick it or leave it, I don't intend to follow up on PRs any more if they break because nobody can be bothered to push a button in time. |
I wonder if adding |
* Use asciidoc from buildPackages, eases cross-platform builds * Add bash to buildDeps for patchShebangs to apply it * Fix wrapProgram shebang in cross-platform builds * Add specific deps for btrbk and ssh filter script to PATH
3372539
to
547b382
Compare
I take this as a mild offense. |
I marked this as stale due to inactivity. → More info |
Motivation for this change
This packages need bash in buildDeps in order to have it patched
correctly by patchShebangs().
In addition, wrapProgram generates the wrong shebang when cross-building
which needs to be corrected manually.
Things done
sandbox
innix.conf
on non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
./result/bin/
)nix path-info -S
before and after)