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

bat-extras: init at 20200408 #85836

Closed
wants to merge 1 commit into from
Closed

bat-extras: init at 20200408 #85836

wants to merge 1 commit into from

Conversation

@bbigras
Copy link
Contributor

bbigras commented Apr 23, 2020

Motivation for this change

Some wrappers tools for bat. Not by the same dev but they are linked on https://github.com/sharkdp/bat.

I don't know if I do chmod u+x for nothing or if there's a way to force wrapProgram even if the sh is not executable.

The --replace '$PAGER' '${less}/bin/less' parts are weird. I tried to replace less with wrapProgram at first but failed.

Things done
  • 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.

/nix/store/zyb128zq9jydyv1l53yjx6bwgw09hwq8-bat-extras-20200408 36.4M

with all optional enabled
/nix/store/a4jav90xic79vn8kgxm3zi51a9zc9dy5-bat-extras-20200408 1.1G

maybe cc the bat maintainers @dywedir @lilyball

@lilyball
Copy link
Member

lilyball commented Apr 23, 2020

I'm not really a fan of the fact that installing bat-extras without any configuration will pull in rustfmt, clang-tools, etc.

It looks like the only one of these dependencies that's actually used during the build process is shfmt, and even that's just to minify the source (I'm not sure what the practical reason is to do this either).

My inclination is to say we should instead expose bat-extras as a collection of derivations, one for each script, so we can put the dependencies on each script separately. prettybat still needs config in this case, but that seems a lot more reasonable than making me pull in rustfmt and clang-tools simply because I wanted batgrep. Each of the nested derivations can wrap a central hidden bat-extras derivation that does the actual build, and the wrappers will then just run wrapProgram as necessary on the script they expose. This means installing multiple scripts will only build the core bat-extras once.

As for shfmt, I'm not sure how lightweight it is; should we just go ahead and allow build.sh to minify using it (since the default is to minify the library scripts) and therefore require it for any of the scripts, or should we disable minification in the build? I just tested, minifying library scripts reduces the resulting bin folder from 84KB to 52KB (and --minify=all takes that to 48KB). I also benchmarked bash -n batgrep (i.e. just parsing the script) and the unminified version is 3.1ms ± 0.4ms with the minified version being 2.6ms ± 0.4ms, so not much of a difference and hardly seems worth bothering. So I'm inclined to say we should --minify=none and avoid depending on shfmt for anything besides prettybat.

cp bin/* $out/bin
substituteInPlace $out/bin/batgrep \
--replace '$PAGER' '${less}/bin/less'

This comment has been minimized.

Copy link
@lilyball

lilyball Apr 23, 2020

Member

This is going to force pager behavior always, when the pager would otherwise not be used. Not only that, but it also disables the normal reading of the PAGER env var.

By that I mean, if I build bat-extras myself and run batgrep with my current environment, it doesn't use a pager, because I don't have any pager-related env vars set up. But with this substitution, it will completely ignore the PAGER env var and always use Nix-installed less, even if I wanted to use something else.

What we do for bat is just inject Nix's less into PATH, so when bat defaults to less (or when PAGER=less) it uses the Nix less (I forget why this was important but I think it works around some bug in macOS less combined with Bat's default options). I'm inclined to say we should just do the same here and wrap each of the bat-extras scripts such that Nix's less is prefixed to PATH. This way PAGER=less batgrep foo will use the same pager that PAGER=less bat foo uses.

wrapProgram .test-framework/lib/print_util.sh \
--prefix PATH : "${ncurses}/bin" \
Comment on lines +30 to +31

This comment has been minimized.

Copy link
@lilyball

lilyball Apr 24, 2020

Member

This seems wrong. This is a library file that's sourced, not an executable. Wrapping it like this means the act of sourcing it will modify the path. Instead we should make ${ncurses}/bin available either in a wrapper of .test-framework/bin/best.sh, or of test.sh itself, or just globally during the build.

@lilyball
Copy link
Member

lilyball commented Apr 24, 2020

It also occurs to me that I don't see anything in here telling bat-extras where to find bat itself.

@lilyball lilyball mentioned this pull request Apr 25, 2020
5 of 10 tasks complete
@lilyball
Copy link
Member

lilyball commented Apr 25, 2020

I got nerd-sniped and ended up rewriting this to my own satisfaction, submitted as #85982.

@lilyball
Copy link
Member

lilyball commented Apr 25, 2020

I left you as a maintainer in that PR but added myself, I hope that's ok.

@bbigras bbigras closed this Apr 29, 2020
@bbigras bbigras deleted the bbigras:bat-extras branch Apr 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.