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

taoup: init at 1.1.14 #136751

Merged
merged 1 commit into from
Sep 23, 2021
Merged

taoup: init at 1.1.14 #136751

merged 1 commit into from
Sep 23, 2021

Conversation

zakame
Copy link
Member

@zakame zakame commented Sep 5, 2021

Motivation for this change

https://github.com/globalcitizen/taoup is a fortune-like tool that tells some words of wisdom from The Art of Unix Programming and other sources.

Some patches are included to simplify the taoup-fortune example script (which tries to produce a cache directory at first run - we instead create and populate this at build time,) and taoup itself (which tries to test for tput - avoid that and just depend on ncurses to provide that instead.)

I suspect some improvements can be made still at the postPatch and installPhase steps - I'd love to hear anyone's guidance on this 🙏 Thanks!

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • 21.11 Release Notes (or backporting 21.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

@ofborg ofborg bot added 8.has: package (new) This PR adds a new package 11.by: package-maintainer This PR was created by the maintainer of the package it changes 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild 10.rebuild-linux: 1-10 10.rebuild-linux: 1 labels Sep 5, 2021
Copy link
Contributor

@Aegyo Aegyo left a comment

Choose a reason for hiding this comment

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

Result of nixpkgs-review pr 136751 run on x86_64-linux 1

1 package built:
  • taoup

taoup --help looks like it has some unintended output:

$ taoup --help
usage: /nix/store/7fyilmppfb39qvc93nv3lc6zqz1p68r1-taoup-1.1.13/lib/taoup/taoup [arguments]
       /nix/store/7fyilmppfb39qvc93nv3lc6zqz1p68r1-taoup-1.1.13/lib/taoup/taoup                    Display all fortunes and sections.
       /nix/store/7fyilmppfb39qvc93nv3lc6zqz1p68r1-taoup-1.1.13/lib/taoup/taoup < --help | -h >    This help.
       /nix/store/7fyilmppfb39qvc93nv3lc6zqz1p68r1-taoup-1.1.13/lib/taoup/taoup --whitetrash       Convert ANSI colors for light/white terminals.
       /nix/store/7fyilmppfb39qvc93nv3lc6zqz1p68r1-taoup-1.1.13/lib/taoup/taoup --machine          Remove ANSI colors.
       /nix/store/7fyilmppfb39qvc93nv3lc6zqz1p68r1-taoup-1.1.13/lib/taoup/taoup --fortune          Convert output to fortune format (and lose colors).

Is the full path expected there? It looks a bit cluttered to me.

I've also commented on a couple of minor things in the code.

I'm new to nix, I hope it is ok for me to review things like this (and potentially have no idea what I am talking about).

This looks like an interesting program, thank you for packaging it :)

pkgs/tools/misc/taoup/default.nix Outdated Show resolved Hide resolved
pkgs/tools/misc/taoup/default.nix Outdated Show resolved Hide resolved
@zakame
Copy link
Member Author

zakame commented Sep 7, 2021

@Aegyo thanks for the review, it is indeed helpful! 🙏

Thanks for pointing out the --help output - sounds like we can patch it to not use $0 in https://github.com/globalcitizen/taoup/blob/83d355ddac16f43ce075a34933f3a76ce027011b/taoup#L10-L15 and just use ${pname}.

@zakame zakame changed the title taoup: init at 1.1.13 taoup: init at 1.1.14 Sep 7, 2021
@zakame
Copy link
Member Author

zakame commented Sep 7, 2021

Result of nixpkgs-review run on aarch64-darwin 1

1 package built:
  • taoup

taoup --help should also look neater now:

(nix-shell) [zakame:~/.cache/nixpkgs-review/rev-2132d813cb0845647ee3cb19906cf8c63ee1e88e] $ taoup --help
usage: taoup [arguments]
       taoup                    Display all fortunes and sections.
       taoup < --help | -h >    This help.
       taoup --whitetrash       Convert ANSI colors for light/white terminals.
       taoup --machine          Remove ANSI colors.
       taoup --fortune          Convert output to fortune format (and lose colors).

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-ready-for-review/3032/592

@zakame
Copy link
Member Author

zakame commented Sep 17, 2021

I expect package builds to fail as #138240 (which brings in rubyPackages.ansi) isn't merged yet.

@SuperSandro2000 SuperSandro2000 merged commit 29d3066 into NixOS:master Sep 23, 2021
@zakame
Copy link
Member Author

zakame commented Sep 28, 2021

Thanks @SuperSandro2000 ! 🙏 🎉

@zakame zakame deleted the contrib/ruby-taoup branch September 28, 2021 06:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
8.has: package (new) This PR adds a new package 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild 10.rebuild-linux: 1-10 10.rebuild-linux: 1 11.by: package-maintainer This PR was created by the maintainer of the package it changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants