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

miniupnpc: 2.2.7 -> 2.2.8, again #326402

Merged
merged 39 commits into from
Jul 12, 2024
Merged

Conversation

emilazy
Copy link
Member

@emilazy emilazy commented Jul 11, 2024

Description of changes

Like #325273, except I migrated like half of the reverse dependencies in the universe with my own bare hands and added upstream patches and updates for the other half. My own patches have been sent off to every upstream that still has even the slightest hint of still being maintained.

Every package that built before still builds now, except for masari, which I dropped with the consent of its maintainer due to being a dead cryptocurrency abandoned by its upstream. Not that there aren’t a lot of packages that did get patched here that aren’t in much better shape.

There were a lot of real staring‐into‐the‐abyss moments in this one. Hell is other people’s build systems. I’m going to bed now.

cc @nagy @K900 @doronbehar

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • 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/)
  • 24.11 Release Notes (or backporting 23.11 and 24.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.

Add a 👍 reaction to pull requests you find important.

Includes a fix for miniupnpc 2.2.8.
Switch to new GitLab repository and pin the last commit.

The upstream repository is archived, so it’s unlikely there’ll
ever be another update.
As it doesn’t look like there’ll be another upstream release,
add bug fix patches from the upstream bug tracker, plus one I wrote
just now for miniupnpc 2.2.8 support.
Dead cryptocurrency; last release was in 2019, last commit was in
2022. This is broken with miniupnpc 2.2.8; I reached out to the
maintainer and we agreed that it’s fine to just drop the package
rather than waste time patching it.
@K900
Copy link
Contributor

K900 commented Jul 12, 2024

You're insane. I just want you to know that.

@@ -1,4 +1,4 @@
{ lib, stdenv, mkDerivation, fetchFromGitHub
{ lib, stdenv, mkDerivation, fetchFromGitHub, fetchpatch2
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is fetchpatch2 used here?

Copy link
Member Author

Choose a reason for hiding this comment

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

fetchpatch is “deprecated”; fetchpatch2 is meant to be better but is backwards‐incompatible, so you have to opt in.

Ironically we probably need a fetchpatch3: #266556

Comment on lines +59 to 60

./use-system-libraries.patch
Copy link
Contributor

Choose a reason for hiding this comment

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

A bit weird to me how these external patches don't allow us to not have also a local patch... However this shouldn't block this PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

The previous anti‐vendoring patch was actually not fully functional in terms of specifying the header locations. The upstream PRs that fix miniupnpc 2.2.8 also forced the vendoring even harder. So since it’ll need dealing with anyway on the next version, I just fixed the anti‐vendoring stuff and reduced its size in the process.

# SSL requires libevent at 2.1 with ssl support
configureFlags = [ "--disable-ssl" ];

__structuredAttrs = true;

passthru.updateScript = nix-update-script { };
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this really needed? Updating this package should be trivial for both nix{,pkgs}-update.

Copy link
Member Author

Choose a reason for hiding this comment

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

Honestly, it’s never been clear to me exactly where nixpkgs-update will update from. I know the main source is Repology, but that it can maybe also do GitHub releases? In this case though the newer releases are just tags and there are no corresponding GitHub releases. This at least lets maintainers/script/update.nix work where it doesn’t otherwise.

I asked about best practices on Matrix recently but there wasn’t really any answer. There has been some talk about either making nix-update-script the default, or doing a mass addition of it; see #325074.

Copy link
Contributor

@doronbehar doronbehar left a comment

Choose a reason for hiding this comment

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

Looks great to me! I trust you have tested every package touched by this.

@K900
Copy link
Contributor

K900 commented Jul 12, 2024

Result of nixpkgs-review pr 326402 run on x86_64-linux 1

6 packages marked as broken and skipped:
  • aeon
  • bitcoin-unlimited
  • bitcoind-unlimited
  • groestlcoin
  • pivx
  • pivxd
66 packages built:
  • aerc
  • alephone
  • alephone.icons
  • atomic-swap
  • bitcoin
  • bitcoin-abc
  • bitcoind
  • bitcoind-abc
  • bitcoind-knots
  • chiaki4deck
  • dante
  • dolphin-emu
  • dolphin-emu-primehack
  • drawpile
  • eiskaltdcpp
  • elements
  • elementsd
  • elementsd-simplicity
  • flycast
  • fragments
  • gridcoin-research
  • groestlcoind
  • haven-cli
  • haven-cli.source
  • hydrus
  • hydrus.doc
  • i2pd
  • libtransmission_3
  • libtransmission_3.apparmor
  • libtransmission_4
  • libtransmission_4.apparmor
  • litecoin
  • litecoind
  • lutris
  • lutris-free
  • miniupnpc
  • monero-cli
  • monero-cli.source
  • monero-gui
  • namecoind
  • particl-core
  • pshs
  • qodem
  • retroshare
  • sunshine
  • swiften
  • torrential
  • transmission_3
  • transmission_3-gtk
  • transmission_3-gtk.apparmor
  • transmission_3-qt
  • transmission_3-qt.apparmor
  • transmission_3.apparmor
  • transmission_3_noSystemd
  • transmission_3_noSystemd.apparmor
  • transmission_4
  • transmission_4-gtk
  • transmission_4-gtk.apparmor
  • transmission_4-qt (transmission_4-qt5)
  • transmission_4-qt.apparmor (transmission_4-qt5.apparmor)
  • transmission_4-qt6
  • transmission_4-qt6.apparmor
  • transmission_4.apparmor
  • warzone2100
  • yaup
  • zeroadPackages.zeroad-unwrapped

@doronbehar doronbehar merged commit 40dc19e into NixOS:master Jul 12, 2024
26 checks passed
Copy link
Member

@nagy nagy left a comment

Choose a reason for hiding this comment

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

Thank you, this is impressive work.

@emilazy
Copy link
Member Author

emilazy commented Jul 12, 2024

Looks great to me! I trust you have tested every package touched by this.

Uhh, I tested that they build at least… 🫠

The breaking API change is just two additional parameters that can be set to NULL, 0 and a renumbering of the return codes, so as long as it builds and any applicable return code checking is updated it should just work. I didn’t try and exercise the functionality of every single package here because there’s way too many of them and “stuff builds” tends to be our rubric for doing updates of libraries anyway. Thankfully a lot of packages had already patched this upstream, so if anything breaks from incorporating those patches it’s on them :)

Of course, I’m happy to be held responsible if any regressions do occur and help out with fixing them, and if there’s any substantive upstream feedback on the patches I wrote I’ll make sure to address them in Nixpkgs as well.

@miniupnp
Copy link

@emilazy sorry for the API break :(

@emilazy
Copy link
Member Author

emilazy commented Jul 23, 2024

Hey, it happens :) Although a larger version bump would probably make it catch fewer people by surprise next time.

@emilazy emilazy deleted the miniupnpc-2.2.8-again branch August 26, 2024 01:04
@emilazy emilazy mentioned this pull request Sep 26, 2024
13 tasks
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.

5 participants