-
-
Notifications
You must be signed in to change notification settings - Fork 13.5k
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
miru: added passthru.updateScript #325352
Conversation
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/prs-ready-for-review/3032/4210 |
pkgs/by-name/mi/miru/update.sh
Outdated
MIRU_LATEST_VER=$(curl -s "https://api.github.com/repos/ThaUnknown/miru/releases/latest" | jq -r .tag_name | sed 's/v//') | ||
MIRU_CURRENT_VER=$(grep -oP 'version = "\K[^"]+' "$PACKAGE_NIX") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MIRU_LATEST_VER=$(curl -s "https://api.github.com/repos/ThaUnknown/miru/releases/latest" | jq -r .tag_name | sed 's/v//') | |
MIRU_CURRENT_VER=$(grep -oP 'version = "\K[^"]+' "$PACKAGE_NIX") | |
MIRU_LATEST_VER="$(curl -s "https://api.github.com/repos/ThaUnknown/miru/releases/latest" | jq -r .tag_name | sed 's/v//')" | |
MIRU_CURRENT_VER="$(grep -oP 'version = "\K[^"]+' "$PACKAGE_NIX")" |
pkgs/by-name/mi/miru/update.sh
Outdated
LINUX_NIX="$ROOT/linux.nix" | ||
DARWIN_NIX="$ROOT/darwin.nix" | ||
|
||
MIRU_LATEST_VER=$(curl -s "https://api.github.com/repos/ThaUnknown/miru/releases/latest" | jq -r .tag_name | sed 's/v//') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should specify that you only want to remove the starting v
(in case the versioning format ever changes we never know)
MIRU_LATEST_VER=$(curl -s "https://api.github.com/repos/ThaUnknown/miru/releases/latest" | jq -r .tag_name | sed 's/v//') | |
MIRU_LATEST_VER=$(curl -s "https://api.github.com/repos/ThaUnknown/miru/releases/latest" | jq -r .tag_name | sed 's/^v//') |
We could use a more robust regex (s/^v([0-9]+\.[0-9]+\.[0-9]+)$/\1
) to be sure that this does not break in case the versioning system changes. But keeping it simple should suffice and will avoid any potential error (and it's more readable), so I will leave it up to you to decide.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Keeping the readable one for now, will revisit if things change upstream
pkgs/by-name/mi/miru/update.sh
Outdated
LINUX_NIX="$ROOT/linux.nix" | ||
DARWIN_NIX="$ROOT/darwin.nix" | ||
|
||
MIRU_LATEST_VER=$(curl -s "https://api.github.com/repos/ThaUnknown/miru/releases/latest" | jq -r .tag_name | sed 's/v//') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please check whether the request returned a valid result (e.g. if the repository's name changes the version tag will be null
, raising a rather unclear error message from nix-prefetch-url
, which could be prevented)
--- SHOWING ERROR LOG FOR miru-5.1.6 ----------------------
error: unable to download 'https://github.com/ThaUnknown/mirud/releases/download/vnull/linux-Miru-null.AppImage': HTTP error 404
response body:
Not Found
error: hash '' has wrong length for hash type 'sha256'
--- SHOWING ERROR LOG FOR miru-5.1.6 ----------------------
The update script for miru-5.1.6 failed with exit code 1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, I'm sorry I unintentionally approved and github won't let me remove my approval 😓
Please address my remarks first.
|
ad4e533
to
27fd1be
Compare
@d4ilyrun comments addressed, retested on |
pkgs/by-name/mi/miru/update.sh
Outdated
MIRU_CURRENT_VER="$(grep -oP 'version = "\K[^"]+' "$PACKAGE_NIX")" | ||
|
||
if [[ "$MIRU_LATEST_VER" == "null" ]]; then | ||
echo "error: could not fetch miru latest version from GitHub API" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The update.py
script only displays the content of stderr when an error occurs.
echo "error: could not fetch miru latest version from GitHub API" | |
echo "error: could not fetch miru latest version from GitHub API" >&2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good to know, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@d4ilyrun fixed
27fd1be
to
7470522
Compare
pkgs/by-name/mi/miru/update.sh
Outdated
|
||
ROOT="$(dirname "$(readlink -f "$0")")" | ||
if [[ ! "$(basename $ROOT)" == "miru" || ! -f "$ROOT/package.nix" ]]; then | ||
echo "ERROR: Not in the miru folder" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops sorry I missed this one, same as the previous error message
echo "ERROR: Not in the miru folder" | |
echo "error: Not in the miru folder" >&2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM after this one !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@matteo-pacini You re-requested a review but did not fix this last issue, was this a mistake?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@d4ilyrun yup, my bad! Forgot to push the changes 😇
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/prs-ready-for-review/3032/4218 |
Does nix-update not work? I thought it handled gh releases. |
7470522
to
14e8bf3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🚀
@eclairevoyant Didn't know it could, hence the custom approach 🙏🏻 |
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/prs-already-reviewed/2617/1828 |
pkgs/by-name/mi/miru/update.sh
Outdated
LINUX_NIX="$ROOT/linux.nix" | ||
DARWIN_NIX="$ROOT/darwin.nix" | ||
|
||
MIRU_LATEST_VER="$(curl -s "https://api.github.com/repos/ThaUnknown/miru/releases/latest" | jq -r .tag_name | sed 's/^v//')" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MIRU_LATEST_VER="$(curl -s "https://api.github.com/repos/ThaUnknown/miru/releases/latest" | jq -r .tag_name | sed 's/^v//')" | |
MIRU_LATEST_VER="$(curl -s ${GITHUB_TOKEN:+-u ":$GITHUB_TOKEN"} "https://api.github.com/repos/ThaUnknown/miru/releases/latest" | jq -r .tag_name | sed 's/^v//')" |
pkgs/by-name/mi/miru/update.sh
Outdated
exit 1 | ||
fi | ||
|
||
if [ "$MIRU_LATEST_VER" = "$MIRU_CURRENT_VER" ]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if [ "$MIRU_LATEST_VER" = "$MIRU_CURRENT_VER" ]; then | |
if [[ "$MIRU_LATEST_VER" == "$MIRU_CURRENT_VER" ]]; then |
please use new test consistently
14e8bf3
to
bea3a3f
Compare
@SuperSandro2000 comments addressed, thanks |
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/prs-already-reviewed/2617/1838 |
@SuperSandro2000 friendly ping so we can get this merged 🙏🏻 thanks! |
ROOT="$(dirname "$(readlink -f "$0")")" | ||
if [[ ! "$(basename $ROOT)" == "miru" || ! -f "$ROOT/package.nix" ]]; then | ||
echo "error: Not in the miru folder" >&2 | ||
exit 1 | ||
fi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nixpkgs-update and r-ryantm will usually call the update script from nixpkgs root folder. You can test this easily with nix-update -u miru
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please note the -u
, which will call the passthru.updateScript
@SuperSandro2000 friendly ping for merge, as this has been alive for 3 weeks now 🙏🏻 |
functions as expected... further updates to 5.2.7 pending |
Description of changes
Added an update script for Miru.
To test it (in my branch):
nix-shell maintainers/scripts/update.nix --argstr package miru
Then build miru again -
nix-build -A miru
- and run it.It should be the latest available version: 5.1.10 at the time of this port.
Things done
nix.conf
? (See Nix manual)sandbox = relaxed
sandbox = true
nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)Add a 👍 reaction to pull requests you find important.