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
kjv: init at unstable-2018-12-25 #64565
Conversation
20b777a
to
c9e6697
Compare
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.
A few suggestions, but i was able to build the package.
./result/bin/kjv --help
usage: kjv [flags] [reference...]
-l list books
-W no line wrap
-h show help
...
@jonringer Thanks for the careful review! I've made all the changes you suggested. |
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.
just a little more cleanup, doesn't change the closure though :)
$ nix-store -q --tree ./result # after
/nix/store/wzdw64avynfad1mlr11sb7qa02v86jdz-kjv
+---/nix/store/7bkv33x88dc64yhjvfvjvh0fqyhlliwj-bash-4.4-p23
+---/nix/store/bjmg1g133m9xwxa0iy5inp2snvnfg151-glibc-2.27
| +---/nix/store/bjmg1g133m9xwxa0iy5inp2snvnfg151-glibc-2.27 [...]
+---/nix/store/7bkv33x88dc64yhjvfvjvh0fqyhlliwj-bash-4.4-p23 [...]
$ nix-store -q --tree ./result # before
/nix/store/av5h5mj4w1cxc0kxm31ndawn4pcrc18g-kjv
+---/nix/store/7bkv33x88dc64yhjvfvjvh0fqyhlliwj-bash-4.4-p23
+---/nix/store/bjmg1g133m9xwxa0iy5inp2snvnfg151-glibc-2.27
| +---/nix/store/bjmg1g133m9xwxa0iy5inp2snvnfg151-glibc-2.27 [...]
+---/nix/store/7bkv33x88dc64yhjvfvjvh0fqyhlliwj-bash-4.4-p23 [...]
You'll have to sit tight until someone with commit access reviews :) |
7d86fcc
to
bb6a77a
Compare
@jb55 Thanks for the review! I kept the raw URL for the homepage, but simplified the repo attribute per your suggestion. |
(Pinging anyone with push access to take another look at this.) |
@marsam do you mind take a look at this? :). Should be ready to go |
@marsam Thanks for the review! I've made most of your suggestions, and left a comment re: packaging the fork. |
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.
See comments.
Also can you change the commit msg Add maintainer info for Jared Tobin.
to
maintainers: add Jared Tobin
@GrahamcOfBorg build kjv |
70ceba6
to
1893cdc
Compare
@worldofpeace Thanks for the review -- I've made the changes you (and, re: avoiding the fork, also @marsam) requested. Just let me know if anything else seems worth patching up. In particular, I added a manual |
@worldofpeace Thanks for following up! I left some comments, but will happily defer to you here if my approach isn't acceptable. |
I'd have to disagree with #64565 (comment). The commit adds an install target LukeSmithxyz/kjv@f4ad735#diff-b67911656ef5d18c4ae36cb6741b7965R16 |
@worldofpeace All good -- I've rebased and added the install target patch 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.
LGTM 👍
I'd also like to mention it's best avoided to start your PR branch directly on master
.
See https://nixos.org/nixpkgs/manual/#submitting-changes-making-patches
Motivation for this change
Simply adds bonibon's
kjv
to nixpkgs.Things done
sandbox
innix.conf
on non-NixOS)nix-shell -p nix-review --run "nix-review wip"
./result/bin/
)nix path-info -S
before and after)