-
-
Notifications
You must be signed in to change notification settings - Fork 14.7k
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
sigrok and friends: Update to nightly #259629
Conversation
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.
I've also asked for any release plans a few weeks ago and received the same answer as you did. Then I also updated the packages locally to their unstable versions, but I never dared to upstream them. I'll compare what I did with your proposal and add more suggestions where useful.
In general, I agree that we should ship unstable packages for now as upstream hasn't made releases for so long.
cbbfe96
to
e499cc9
Compare
Hey @panicgh, thank you kindly for your review(s)! I made updated all the versions to what I am still unsure about the |
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.
I leave the decision whether the version should be "nightly" or "unstable" to the maintainer or whoever merges the PR.
LGTM. Tested pulseview with a logic analyzer and smuview with a multimeter.
Result of nixpkgs-review pr 259629
run on x86_64-linux 1
9 packages built:
- collectd
- libsigrok
- libsigrokdecode
- pulseview
- python310Packages.sigrok
- python311Packages.sigrok
- sigrok-cli
- sigrok-firmware-fx2lafw
- smuview
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/prs-already-reviewed/2617/1162 |
5b9ff21
to
205e2ee
Compare
205e2ee
to
033afb3
Compare
Commit 4b030f5a42c65eb3c36361c058c34846a36f8376 does not seem to belong into this PR. |
033afb3
to
dcb9c87
Compare
fab68c5
to
3280721
Compare
3280721
to
14dcbd0
Compare
Thanks for the ping @panicgh! Was at a good time. :) Rebased, removed the substitution in the url (even though I don't necessarily agree) and updated to the latest commits. |
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.
Result of nixpkgs-review pr 259629
run on x86_64-linux 1
9 packages built:
- collectd
- libsigrok
- libsigrokdecode
- pulseview
- python311Packages.sigrok
- python312Packages.sigrok
- sigrok-cli
- sigrok-firmware-fx2lafw
- smuview
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/prs-already-reviewed/2617/1404 |
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.
version
attributes should be <latest version>-unstable-<YYYY-MM-DD>
, see /pkgs/README.md#package-naming.
And the new smuview
should ideally be under the new /pkgs/by-name
. To make that work, in addition to moving the file, you'll need to use qt5
to get wrapQtAppsHook
.
While this is what the file says, it seems very unpopular in the repository as is and also the nixpkgs doc contains an example just using a "unstable-<yyyy-mm-dd>" version (the only example with unstable version!). $ git grep -l 'version.*=.*"[[:digit:]].*unstable' pkgs/*.nix | wc -l
103
$ git grep -l 'version.*=.*"unstable' pkgs/*.nix | wc -l
1518 I, therefore, favor finally merging these changes and let someone else cleanup the unstable versions nixpkgs-wide and in the documentation if it is regarded as relevant, or change the readme to reflect the widely used version name scheme. |
Disagree.
I see none of these points as good reasons for not fixing this here & now, before it comes more annoying to properly address this post-merge. If the only leftover issue is that you don't want this to go unmerged for an eternity: Since I'm currently using sigrok stuff for university work, I'll gladly stay on the ball w/r/t reviews and merge this - once I'm happy with its guideline compliance. |
I wouldn't mind fixing this PR if we weren't gambling with the author's time and patience, who is waiting for 4 months to get it merged. It was originally intended for 23.11 and created well due in time. Still the improvements are not yet merged (the upstream releases are from 2019-2021, massively outdated). I could help to make this pkg comply to the latest guideline and bump it from time to time, but as long as this PR isn't merged I cannot do anything. I am happy about your offer to keep an eye on this pkg. |
Hey, just bumping this for you @vifino. (as I am also interested in seeing this merged in) If I had any say in the discussion, I'd vote for making the (seemingly trivial) changes to conform to the guidelines so that we could see this in master soon! |
I do not understand the purpose of insisting on dragging a version number around if it bears no significance anymore. I understand that scheme when a specific unreleased "hot fix" commit was chosen and the project intends to cut another release in the future. But if the version is years behind and the project has no plans to release a new version, that number is meaningless. Besides, this is a should, not a must. Why must this be enforced here? Regardless of what I think on this issue, after more than 4 months of keeping this up to date and addressing feedback - including the change from I will update this PR one last time. |
14dcbd0
to
881fdbc
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.
Result of nixpkgs-review pr 259629
run on x86_64-linux 1
9 packages built:
- collectd
- libsigrok
- libsigrokdecode
- pulseview
- python311Packages.sigrok
- python312Packages.sigrok
- sigrok-cli
- sigrok-firmware-fx2lafw
- smuview
(also built everything on aarch64-linux, but can't run nixpkgs-review
there)
Checked pulseview
with the logic analyser at my uni's lab, and sigrok-cli
gave the output I'm used to from Debian. LGTM.
Thank you. |
Description of changes
The Sigrok project hasn't had a release in a long time and there is no timeline on when that happens.
However, development is far from stopped.
On the project's site, it is recommended to use the nightly builds.
I asked on IRC about it and it was recommended to package nightly releases until the project publishes releases again.
This advances the packaged version roughly two years or more.
All in one PR, as it doesn't make sense to update one or the other.
In addition, I packaged smuview, which is also "a sigrok project", even though it doesn't use the sigrok.org Git.
Things done
sandbox = true
set innix.conf
? (See Nix manual)nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)