Skip to content
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

Jump to bottom

dasht: init at v2.3.0 #56099

Merged
merged 2 commits into from Feb 23, 2019
Merged

dasht: init at v2.3.0 #56099

merged 2 commits into from Feb 23, 2019

Conversation

matthiasbeyer
Copy link
Contributor

Closes #55546

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

Works for me, I downloaded the bash docset and ran a query on it...

@matthiasbeyer
Copy link
Contributor Author

@symphorien I just pushed my current version (fixup commit) but as far as I can tell, the postInstall phase is never run... I don't know why though.

@symphorien
Copy link
Member

because you override the installPhase without runHook postInstall.
Move what I suggested from postInstall to installPhase, then. My bad.

@matthiasbeyer
Copy link
Contributor Author

Works now... nice! Thank you!

@symphorien
Copy link
Member

Some more nitpicks:

  • add coreutils, sed and so on to deps so that dasht can work in an empty PATH
  • move postInstall to installPhase, but keep runHook postInstall and add a runHook preInstall. The hooks are designed for easy overridability.
  • maybe consider adding yourself to maintainers, given Revert "Remove maintainership" #56101
  • please fix the ofborg failure

@matthiasbeyer
Copy link
Contributor Author

Will do, but tomorrow. Good night and thanks again! ❤️

@matthiasbeyer
Copy link
Contributor Author

Fixed, feel free to ACK, I'll do a squash then!

marsam
marsam requested changes Feb 21, 2019
View reviewed changes
pkgs/tools/misc/dasht/default.nix Outdated Show resolved Hide resolved
pkgs/tools/misc/dasht/default.nix Outdated Show resolved Hide resolved
pkgs/tools/misc/dasht/default.nix Outdated
cp bin/* $out/bin/

mkdir -p $out/share/man
cp man/man1/* $out/share/man/
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please use man1 subdir

--- c/pkgs/tools/misc/dasht/default.nix
+++ i/pkgs/tools/misc/dasht/default.nix
@@ -32,8 +32,8 @@ stdenv.mkDerivation rec {
     mkdir -p $out/bin
     cp bin/* $out/bin/
 
-    mkdir -p $out/share/man
-    cp man/man1/* $out/share/man/
+    mkdir -p $out/share/man/man1
+    cp man/man1/* $out/share/man/man1
 
     for i in $out/bin/*; do
       echo "Wrapping $i"

pkgs/tools/misc/dasht/default.nix Outdated Show resolved Hide resolved
marsam
marsam requested changes Feb 21, 2019
View reviewed changes
pkgs/tools/misc/dasht/default.nix Outdated Show resolved Hide resolved
@symphorien
Copy link
Member

dasht still misses grep when executed in an empty PATH.
Apart from this detail, looks good to me.

@matthiasbeyer
dasht: init at v2.3.0
e77902c 
Signed-off-by: Matthias Beyer <mail@beyermatthias.de>
@matthiasbeyer
Copy link
Contributor Author

Fixed and rebase-squashed.

@symphorien
Copy link
Member

thanks :)

@ryantm
Copy link
Member

ryantm commented Feb 23, 2019

@GrahamcOfBorg build dasht

@ryantm ryantm merged commit 309c6d3 into NixOS:master Feb 23, 2019
@ryantm
Copy link
Member

ryantm commented Feb 23, 2019

Sorry, I meant to squash this to one commit when I merged it, but I guess the GitHub interface tricked me.

@matthiasbeyer matthiasbeyer deleted the add-dasht branch November 9, 2019 16:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marsam marsam marsam approved these changes

@symphorien symphorien symphorien approved these changes

Assignees
No one assigned
Labels
8.has: package (new) 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 11.by: package-maintainer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@matthiasbeyer @symphorien @ryantm @marsam @GrahamcOfBorg