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 #85982

Merged
merged 1 commit into from May 2, 2020
Merged

Conversation

lilyball
Copy link
Member

Motivation for this change

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

This is a rewrite of #85836 that fixes all the issues I identified. I kind of got nerd-sniped here. This version exposes each script as a separate attribute nested under bat-extras. This way you can install e.g. just batgrep without pulling in prettybat and all its dependencies.

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.

@bbigras
Copy link
Contributor

bbigras commented Apr 25, 2020

I can't build. I tested with nix-review and manually.

❯ nix-build -A bat-extras
these derivations will be built:
  /nix/store/1gxi6iqpf7y743grqr8gg4z6y7i3dw97-bat-extras-20200408.drv
  /nix/store/0yhja69mi61xw49xm2gw313z4dpn54c6-bat-extras-batgrep-20200408.drv
  /nix/store/cknfmj8xfqgf7g6i4sscpwl3aqd2njgr-bat-extras-batgrep-20200408.drv
  /nix/store/f5kiyrbmmjvxnpgbkp8dnncmm5i4cbfm-bat-extras-batgrep-20200408.drv
  /nix/store/mwbza033kvnxclgvr04w71pfqfqrkc1r-bat-extras-batgrep-20200408.drv
building '/nix/store/1gxi6iqpf7y743grqr8gg4z6y7i3dw97-bat-extras-20200408.drv'...
unpacking sources
unpacking source archive /nix/store/mp8aadl3sfy1lx8w1i0dk7gipr1wv1qr-source
source root is source
patching sources
patching script interpreter paths in test.sh .test-framework/bin/best.sh
test.sh: interpreter directive changed from "/usr/bin/env bash" to "/nix/store/z1l2n01xdfcm9bkkir83c32mkpvv51zq-bash-4.4-p23/bin/bash"
.test-framework/bin/best.sh: interpreter directive changed from "/usr/bin/env bash" to "/nix/store/z1l2n01xdfcm9bkkir83c32mkpvv51zq-bash-4.4-p23/bin/bash"
building
This will not install the script.
Use --install for a global install.

Preparing scripts...
Building scripts...
    [1/4] /build/source/src/batgrep.sh
          Reading...
          Preprocessing...
          Minifying...
          Compressing [skipped]
          Building...
          Installing [skipped]
    [2/4] /build/source/src/batman.sh
          Reading...
          Preprocessing...
          Minifying...
          Compressing [skipped]
          Building...
          Installing [skipped]
    [3/4] /build/source/src/batwatch.sh
          Reading...
          Preprocessing...
          Minifying...
          Compressing [skipped]
          Building...
          Installing [skipped]
    [4/4] /build/source/src/prettybat.sh
          Reading...
          Preprocessing...
          Minifying...
          Compressing [skipped]
          Building...
          Installing [skipped]
running tests
--------------------------------------------------------------------------------
| Test Suite: lib_pager                                                        |
--------------------------------------------------------------------------------
[FAIL] env_bat_pager        :: Exited with code 126.
[FAIL] less_args            :: Exited with code 126.
[FAIL] less_detection       :: Expectation failed: [ less_but_renamed = less ]
[FAIL] less_version         :: Expectation failed: [ 0 = 473 ]
 ....                       :: Expectation failed: [ 0 = 551 ]

Totals:
    PASS: 0 / 4
    FAIL: 4 / 4
--------------------------------------------------------------------------------
| Test Suite: lib_opt                                                          |
--------------------------------------------------------------------------------
[PASS] long                 :: 3 ms
[PASS] long_value_explicit  :: 4 ms
[PASS] long_value_implicit  :: 3 ms
[PASS] short_value_explicit :: 4 ms
[PASS] short_value_implicit :: 4 ms

Totals:
    PASS: 5 / 5
    FAIL: 0 / 5

ALL TESTS PASSED.
--------------------------------------------------------------------------------
| Test Suite: lib_version                                                      |
--------------------------------------------------------------------------------
[PASS] compare_eq           :: 5 ms
[PASS] compare_ge           :: 5 ms
[PASS] compare_gt           :: 5 ms
[PASS] compare_le           :: 6 ms
[PASS] compare_lt           :: 4 ms
[PASS] compare_ne           :: 4 ms

Totals:
    PASS: 6 / 6
    FAIL: 0 / 6

ALL TESTS PASSED.
builder for '/nix/store/1gxi6iqpf7y743grqr8gg4z6y7i3dw97-bat-extras-20200408.drv' failed with exit code 2
cannot build derivation '/nix/store/0yhja69mi61xw49xm2gw313z4dpn54c6-bat-extras-batgrep-20200408.drv': 1 dependencies couldn't be built
error: build of '/nix/store/0yhja69mi61xw49xm2gw313z4dpn54c6-bat-extras-batgrep-20200408.drv', '/nix/store/cknfmj8xfqgf7g6i4sscpwl3aqd2njgr-bat-extras-batgrep-20200408.drv', '/nix/store/f5kiyrbmmjvxnpgbkp8dnncmm5i4cbfm-bat-extras-batgrep-20200408.drv', '/nix/store/mwbza033kvnxclgvr04w71pfqfqrkc1r-bat-extras-batgrep-20200408.drv' failed

@lilyball
Copy link
Member Author

lilyball commented Apr 25, 2020 via email

@bbigras
Copy link
Contributor

bbigras commented Apr 26, 2020

x86_64

@lilyball
Copy link
Member Author

lilyball commented Apr 26, 2020 via email

@bbigras
Copy link
Contributor

bbigras commented Apr 26, 2020

Oh sorry, NixOS.

@lilyball
Copy link
Member Author

I've fixed the library tests. batman is still failing, which I'll investigate tomorrow.


# Patch shebangs now because our tests rely on them
postPatch = ''
patchShebangs --host bin/${name}
Copy link
Member Author

Choose a reason for hiding this comment

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

Oops this is actually wrong, I should be passing --build and then allowing the normal fixup patch after all, I think. I’m not actually sure how check phases are supposed to work in cross-compile situations, but if we are cross-compiling we need to test with the build environment and then switch to the host.

Copy link
Member Author

Choose a reason for hiding this comment

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

That is, assuming patchShebangs won’t just ignore any shebangs already pointing to the nix store. Guess I’ll find out tomorrow.

Copy link
Member Author

Choose a reason for hiding this comment

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

Looks like patchShebangs won't patch anything already pointing into the nix store. I guess I'm not sure the right solution here. I'll just leave it as it is for now since this means the installed output will be patched for the host. Alternatively we could install, then run check using the uninstalled bits, which will allow us to patch the install separately, but I think that's getting a bit complicated when I don't even know if check phases run when cross-compiling.

@lilyball
Copy link
Member Author

batman and prettybat tests bypassed in NixOS, they seem to be broken upstream there (eth-p/bat-extras#19).

@bbigras
Copy link
Contributor

bbigras commented Apr 26, 2020

Result of nixpkgs-review pr 85982 1

4 packages built:
- bat-extras.batgrep
- bat-extras.batman
- bat-extras.batwatch
- bat-extras.prettybat

@bbigras
Copy link
Contributor

bbigras commented Apr 29, 2020

I tested running the commands. Sorry for the delay.

batman works but it prints the whole man page (it's like it's not using less).

prettybat seems fine using all the formatter.

❯ ./batgrep -t toml reqwest ~/dev/
/nix/store/g9skvj9cvyyn837k04gghhgrvwi8z84f-bat-extras-batgrep-20200408/bin/.batgrep-wrapped: line 137: less -R: command not found
❯ ./batwatch ~/tmp/bleh.sh 
/nix/store/klpfbc1m2s2xmvr79k0x0l644yfdrakq-bat-extras-batwatch-20200408/bin/.batwatch-wrapped: line 359: less -R: command not found

@lilyball
Copy link
Member Author

PAGER='less -R' failing is an issue with bat-extras. It's treating the env var as a command name, not as a shell string.

batman doesn't invoke the pager directly, but it does use it to construct the BAT_PAGER env var for bat to use. Unfortunately since the script is interpreting PAGER it still isn't handling less -R correctly so it's effectively doing BAT_PAGER="'less -R'", and bat handles a garbage pager by disabling paging.

Which is to say, if you set PAGER=less then things should page correctly. If you clear PAGER entirely then paging should mostly work except batgrep doesn't page at all if PAGER or BAT_PAGER isn't set.

All of this is upstream functionality, not a consequence of this package. The behavior differs from #85836 in that that PR blindly overwrite PAGER to be less for batgrep and batwatch (but would be expected to have the same issue with batman).

@lilyball
Copy link
Member Author

I filed the upstream issue as eth-p/bat-extras#24

@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-already-reviewed/2617/140

@eth-p
Copy link

eth-p commented May 1, 2020

Before you merge it, would there be an issue if I changed the version format to YYYY.MM.DD instead of YYYYMMDD as part of the next release (tomorrow)? I had planned on doing it since the last release, but I don't want to cause any issues with package managers.

A couple of other changes to note in that upcoming release:

  • The tput command is no longer required for anything after building. I replaced it with stty size
  • I added a batdiff script that optionally uses delta for printing diffs.

@lilyball
Copy link
Member Author

lilyball commented May 2, 2020

AFAIK the actual format of the version string is fairly irrelevant as far as Nix is concerned. When nix-env checks for upgrades I think it just checks if the version is different, not if it's considered newer, because the assumption is whatever's in the nixpkgs channel is the latest known version.

@DamienCassou DamienCassou merged commit 9c60228 into NixOS:master May 2, 2020
@lilyball lilyball deleted the bat-extras branch May 2, 2020 21:27
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