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

lesspipe: add missing dependency on 'file' #43985

Merged
merged 3 commits into from Jul 30, 2018

Conversation

snaar
Copy link
Contributor

@snaar snaar commented Jul 22, 2018

Motivation for this change

lesspipe uses 'file' internally and doesn't work if your system does not have 'file' pulled in by some other means

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)
  • Fits CONTRIBUTING.md.

@Mic92
Copy link
Member

Mic92 commented Jul 23, 2018

makeWrapper is required to extend the PATH of lesspipe to have file available at runtime:

makeWrapper ${env}/bin/ronn $out/bin/ronn \

file is mentioned as required program in lesspipe, so this should be fine: https://github.com/wofr06/lesspipe

@snaar
Copy link
Contributor Author

snaar commented Jul 23, 2018

Thank you for help. Using "--pure" with nix-shell also uncovered dependency on tput from ncurses, which I added.

I still need help though: while it works now, lesspipe.sh tries to print helpful error message referencing to itself and ends up printing something like:

$ ./result/bin/lesspipe.sh 
LESSOPEN="|/nix/store/b910wzmz5xqfv0i9xapwls8ira5vi0kh-lesspipe-1.82/bin/.lesspipe.sh-wrapped %s"
export LESSOPEN

It should instead refer to the unwrapped version of itself. What is the typical way to handle it? One way I can see right now is to do patching of the lesspipe.sh itself to replace references to 'file' with references to '${file}/bin/file' instead of the wrapProgram step. That seems somewhat fragile. Is there any other canonical way?

@Mic92
Copy link
Member

Mic92 commented Jul 23, 2018

Since it seems to be only used there https://github.com/wofr06/lesspipe/blob/lesspipe/lesspipe.sh#L50,
you could create a simple patch that replaces file and tput there with the help of substituteAll: https://github.com/NixOS/nixpkgs/blob/master/pkgs/development/tools/flatpak-builder/default.nix#L78 https://github.com/NixOS/nixpkgs/blob/master/pkgs/development/tools/flatpak-builder/fix-paths.patch

This is more robust on updates then just using sed.

@snaar
Copy link
Contributor Author

snaar commented Jul 24, 2018

I'm happy with the result now, thank you.

Copy link
Member

@matthewbauer matthewbauer left a comment

Choose a reason for hiding this comment

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

Because this is a shell script, I think it will be much easier to maintain if you just do wrapProgram in postInstall with those paths.

@Mic92 Mic92 merged commit c5d1f77 into NixOS:master Jul 30, 2018
@snaar
Copy link
Contributor Author

snaar commented Jul 30, 2018

What are the procedures for getting a change into current release branch by the way? I'd like to try it for this change just to see how that works.

@Mic92
Copy link
Member

Mic92 commented Jul 31, 2018

# branch of from the release branch
$ git checkout release-18.03
# create a new local branch based on that
$ git checkout -b less-pipe-backport
# cherry pick the commits that were merged in nixpkgs master
$ git cherry-pick -x <commits-from-master...>
# upload to your own nixpkgs fork
$ git push origin less-pipe-backport

and then open a pull request for your less-pipe-backport branch.

@snaar snaar deleted the fix-lesspipe-dependency branch August 1, 2018 02:56
@snaar
Copy link
Contributor Author

snaar commented Aug 1, 2018

Ah ok, no special procedures then. Thanks again.

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

5 participants