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

sigrok and friends: Update to nightly #259629

Merged
merged 6 commits into from
Feb 10, 2024
Merged

Conversation

vifino
Copy link
Member

@vifino vifino commented Oct 7, 2023

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

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 23.11 Release Notes (or backporting 23.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Copy link
Contributor

@panicgh panicgh left a 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.

@vifino vifino force-pushed the sigrok-nightly branch 2 times, most recently from cbbfe96 to e499cc9 Compare October 9, 2023 17:20
@vifino
Copy link
Member Author

vifino commented Oct 9, 2023

Hey @panicgh, thank you kindly for your review(s)!

I made updated all the versions to what git show -s --format=nightly-%cs showed me and removed the unused dependencies in my smuview derivation you mentioned .

I am still unsure about the util-linux and friends, but I'll remove them as well, after all, the linker doesn't complain about missing symbols.

Copy link
Contributor

@panicgh panicgh left a 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

@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/1162

@panicgh
Copy link
Contributor

panicgh commented Oct 26, 2023

Commit 4b030f5 does not seem to belong into this PR.

@vifino
Copy link
Member Author

vifino commented Jan 15, 2024

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.

Copy link
Contributor

@panicgh panicgh left a 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

@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/1404

Copy link
Contributor

@OPNA2608 OPNA2608 left a 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.

@panicgh
Copy link
Contributor

panicgh commented Jan 28, 2024

version attributes should be <latest version>-unstable-<YYYY-MM-DD>, see /pkgs/README.md#package-naming.

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.

@OPNA2608
Copy link
Contributor

it seems very unpopular in the repository
I […] favor […] [letting] someone else cleanup the unstable versions nixpkgs-wide

Disagree.

  1. The change to our guidelines was made with no real announcements afew months ago (coding-conventions: include the preceding upstream version #234201) so many people likely didn't know that it was eventually made, but the details of its formatting have been discussed for a long time. It's starting to be used in recent man-made inits/bumps.
  2. Other documentation is simply outdated when it doesn't use the new guideline. (But I will update the example you pointed out, for consistency's sake 😉)
  3. "It's comparatively unpopular" also applies to the new /pkgs/by-name format - 1092 instances of /pkgs/by-name/fo/foobar/package.nix pattern, vs the entire rest of Nixpkgs.
  4. "I'll just let someone else clean this up" just adds more work and points-of-conflict for whoever "wants" (read: feels most annoyed by non-compliant code) to fix the entirety of Nixpkgs.

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.

@panicgh
Copy link
Contributor

panicgh commented Jan 28, 2024

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).
While I'd welcome a guideline compliant update from the author, I can honestly understand if they gave up on it. If I was them and had not given up, I would do only what is necessary to get over the compliance issues, and not rebase or bump it since that invokes another round of reviews and related delays. This could be done in a follow-up PR.

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.

@carlossless
Copy link
Contributor

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!

@SuperSandro2000 SuperSandro2000 removed their request for review February 6, 2024 15:49
@vifino
Copy link
Member Author

vifino commented Feb 8, 2024

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.
In that case, it is quite sane and useful.

But if the version is years behind and the project has no plans to release a new version, that number is meaningless.
When I see stuff like 0-unstable I get infuriated.

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 nightly-YYYY-MM-DD to unstable-YYYY-MM-DD (against explicit upstreams wishes) - my willingness to put further effort into this is quite low.

I will update this PR one last time.

Copy link
Contributor

@OPNA2608 OPNA2608 left a 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.

@OPNA2608 OPNA2608 merged commit 47072d1 into NixOS:master Feb 10, 2024
22 of 23 checks passed
@vifino
Copy link
Member Author

vifino commented Feb 10, 2024

Thank you.

@vifino vifino deleted the sigrok-nightly branch February 10, 2024 11:24
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

9 participants