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: Add update script #104072

Closed
wants to merge 2 commits into from
Closed

bat: Add update script #104072

wants to merge 2 commits into from

Conversation

NeQuissimus
Copy link
Member

@NeQuissimus NeQuissimus commented Nov 17, 2020

Motivation for this change
  • Add update script
    • There is no good way to get cargoSha256, so the update script is a little complex.
  • Add test
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.

}

oldVersion="$(nix-instantiate --eval -E "with import ./. {}; lib.getVersion ${pname}" | tr -d '"')"
latestTag="$(git -c 'versionsort.suffix=-' ls-remote --exit-code --refs --sort='version:refname' --tags git@github.com:sharkdp/bat.git '*' | tail --lines=1 | cut --delimiter='/' --fields=3 | sed 's|^v||g')"
Copy link
Member

Choose a reason for hiding this comment

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

Would it be easy to reuse something from fetchFromGitHub. Also ssh links only work when the people have working ssh to github. Https works for everyone.

oldVersion="$(nix-instantiate --eval -E "with import ./. {}; lib.getVersion ${pname}" | tr -d '"')"
latestTag="$(git -c 'versionsort.suffix=-' ls-remote --exit-code --refs --sort='version:refname' --tags git@github.com:sharkdp/bat.git '*' | tail --lines=1 | cut --delimiter='/' --fields=3 | sed 's|^v||g')"

if [ ! "$oldVersion" = "$latestTag" ]; then
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if [ ! "$oldVersion" = "$latestTag" ]; then
if [ "$oldVersion" != "$latestTag" ]; then

update-source-version ${pname} "$latestTag" --version-key=version --print-changes
nixpkgs="$(git rev-parse --show-toplevel)"
default_nix="$nixpkgs/pkgs/tools/misc/bat/default.nix"
dummy_cargo="19vhhxfyx3nrngcs6dvwldnk9h4lvs7xjkb31aj1y0pyawz882h9"
Copy link
Member

Choose a reason for hiding this comment

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

Can we use only 000 to make sure that we don't use this one by accident?

Copy link
Member

Choose a reason for hiding this comment

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

This could be

Suggested change
dummy_cargo="19vhhxfyx3nrngcs6dvwldnk9h4lvs7xjkb31aj1y0pyawz882h9"
dummy_cargo="${stdenv.lib.fakeSha256}"

Copy link
Member

Choose a reason for hiding this comment

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

In fact, the "dummy cargo" hash right now looks to be the current hash, meaning as written this won't work if the machine doing the updating has already built this derivation.

old_cargo=$(grep '^\s*cargoSha256 = ' pkgs/tools/misc/bat/default.nix | awk -F= '{print $2}' | tr -d '" ;')
sed -i "s|$old_cargo|$dummy_cargo|g" "$default_nix"
new_cargo=$(nix-build -A bat 2>&1 | grep sha256 | grep got | awk -F: '{print $3}')
sed -i "s|$dummy_cargo|$new_cargo|g" "$default_nix"
Copy link
Member

@Mic92 Mic92 Nov 17, 2020

Choose a reason for hiding this comment

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

nix-update can also handle this with less code. It knows about github releases and cargoSha256

Copy link
Member

@lilyball lilyball left a comment

Choose a reason for hiding this comment

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

@ryantm's bot @r-ryantm seems to know how to update cargo packages already using nixpkgs-update, as evidenced by #93033. Is there some reason we can't let it handle bat as well? I'm not sure what sort of delay there is when letting it deal with packages.

Or alternatively, is there any way to hook into its logic for the updateScript? I don't know if nixpkgs-update is available in any way from nixpkgs. Maybe once nix flakes is stable?

Comment on lines +37 to +38
updateScript = writeScript "update.sh" ''
#!${stdenv.shell}
Copy link
Member

Choose a reason for hiding this comment

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

If you use writeShellScript instead you can avoid the shebang line and Nix will syntax-check it for you.

Suggested change
updateScript = writeScript "update.sh" ''
#!${stdenv.shell}
updateScript = writeShellScript "update.sh" ''

update-source-version ${pname} "$latestTag" --version-key=version --print-changes
nixpkgs="$(git rev-parse --show-toplevel)"
default_nix="$nixpkgs/pkgs/tools/misc/bat/default.nix"
dummy_cargo="19vhhxfyx3nrngcs6dvwldnk9h4lvs7xjkb31aj1y0pyawz882h9"
Copy link
Member

Choose a reason for hiding this comment

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

This could be

Suggested change
dummy_cargo="19vhhxfyx3nrngcs6dvwldnk9h4lvs7xjkb31aj1y0pyawz882h9"
dummy_cargo="${stdenv.lib.fakeSha256}"

Comment on lines +61 to +62
old_cargo=$(grep '^\s*cargoSha256 = ' pkgs/tools/misc/bat/default.nix | awk -F= '{print $2}' | tr -d '" ;')
sed -i "s|$old_cargo|$dummy_cargo|g" "$default_nix"
Copy link
Member

Choose a reason for hiding this comment

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

Surely we can do this with one sed command instead of trying to read the old cargo value out and then replacing it? The hash only shows up once in the file so it's not like we need to replace it multiple times.

update-source-version ${pname} "$latestTag" --version-key=version --print-changes
nixpkgs="$(git rev-parse --show-toplevel)"
default_nix="$nixpkgs/pkgs/tools/misc/bat/default.nix"
dummy_cargo="19vhhxfyx3nrngcs6dvwldnk9h4lvs7xjkb31aj1y0pyawz882h9"
Copy link
Member

Choose a reason for hiding this comment

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

In fact, the "dummy cargo" hash right now looks to be the current hash, meaning as written this won't work if the machine doing the updating has already built this derivation.

@ryantm
Copy link
Member

ryantm commented Nov 17, 2020

I don't see anything about the way bat is written that would stop it from getting updated by r-ryantm. The last time it tried to update it was in May and it stopped early because there was already an open PR for the update.

This update script looks more complicated than necessary. Can we look for other Rust packages that already have updatescripts? https://github.com/Mic92/nix-update/ supports rust updates by the way.

@NeQuissimus NeQuissimus deleted the bat_update branch November 18, 2020 15:05
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