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

add ufetch-nixos package #91428

Closed
wants to merge 4 commits into from
Closed

add ufetch-nixos package #91428

wants to merge 4 commits into from

Conversation

vulcoes
Copy link

@vulcoes vulcoes commented Jun 24, 2020

Motivation for this change

ufetch was not available in nixpkgs.

Things done

added ufetch-nixos to nixpkgs

  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • 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 nixpkgs-review --run "nixpkgs-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)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@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-in-distress/3604/30

Copy link
Contributor

@puzzlewolf puzzlewolf left a comment

Choose a reason for hiding this comment

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

@vulcoes thanks for opening your first PR!

Is there a particular reason to copy the source into nixpgks and not pull it from upstream https://gitlab.com/jschx/ufetch?


meta = with stdenv.lib; {
description = "Tiny system info for NixOS";
homepage = "https://gitlab.com/jschx/-/blob/master/ufetch-nixos";
Copy link
Contributor

Choose a reason for hiding this comment

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

That URL returns a 404, did you mean "https://gitlab.com/jschx/ufetch"?

Copy link
Author

Choose a reason for hiding this comment

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

Hey there, thanks!
There were a few reasons: ufetch at the time I submitted the PR was hardcoded to be the crux script, which I found confusing when I ran it on my machine.
The source has a #!/bin/sh shebang in each script, which to my understanding breaks in NixOS so I modified it.
Lastly, I just didn’t know how to do it since it’s my first PR to a package manager, let alone NixOS.

I can update that URL, no problem.

Copy link
Contributor

Choose a reason for hiding this comment

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

You can use patchShebangs function to patch shebangs to a bash in Nix store. But by default it is done automatically for all executable files in package:

# This setup hook causes the fixup phase to rewrite all script
# interpreter file names (`#! /path') to paths found in $PATH. E.g.,
# /bin/sh will be rewritten to /nix/store/<hash>-some-bash/bin/sh.
# /usr/bin/env gets special treatment so that ".../bin/env python" is
# rewritten to /nix/store/<hash>/bin/python. Interpreters that are
# already in the store are left untouched.
# A script file must be marked as executable, otherwise it will not be
# considered.
fixupOutputHooks+=(patchShebangsAuto)


stdenv.mkDerivation rec {
name = "ufetch-nixos";
src = ./ufetch-nixos;
Copy link
Contributor

Choose a reason for hiding this comment

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

Use fetchFromGitLab on the repository or fetchurl on https://gitlab.com/jschx/ufetch/-/raw/v0.2/ufetch-nixos.

phases = "installPhase";

installPhase = ''
mkdir -p $out
Copy link
Contributor

Choose a reason for hiding this comment

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

The script should be installed to $out/bin so that installing the package adds it on path.

You can use install command install -D $src $out/bin/ufetch, which will create the directory at the same time.


meta = with stdenv.lib; {
description = "Tiny system info for NixOS";
homepage = "https://gitlab.com/jschx/-/blob/master/ufetch-nixos";
Copy link
Contributor

Choose a reason for hiding this comment

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

You can use patchShebangs function to patch shebangs to a bash in Nix store. But by default it is done automatically for all executable files in package:

# This setup hook causes the fixup phase to rewrite all script
# interpreter file names (`#! /path') to paths found in $PATH. E.g.,
# /bin/sh will be rewritten to /nix/store/<hash>-some-bash/bin/sh.
# /usr/bin/env gets special treatment so that ".../bin/env python" is
# rewritten to /nix/store/<hash>/bin/python. Interpreters that are
# already in the store are left untouched.
# A script file must be marked as executable, otherwise it will not be
# considered.
fixupOutputHooks+=(patchShebangsAuto)

name = "ufetch-nixos";
src = ./ufetch-nixos;

phases = "installPhase";
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to be a list but it is not recommended to remove phases this way because it removes more than necessary. In this case you are missing fixupPhase so patchShebangsAuto is not run. See #28910.

@SuperSandro2000
Copy link
Member

CLosing cause review feedback is not addressed since months and I don't think this package really benefits from being in Nix. Just download the script and execute it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants