qbittorrent: 4.5.5 -> 4.6.0 + other changes

[paveloom]

Description of changes

• https://github.com/qbittorrent/qBittorrent/blob/release-4.6.0/Changelog
• Switched to CMake and Qt6
• Fixed the path to the library in the CMake module of libtorrent-rasterbar

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:
  • NixOS test(s) (look inside nixos/tests)
  • and/or package tests
  • or, for functions and "core" functionality, tests in lib/tests or pkgs/test
  • made sure NixOS tests are linked to the relevant packages
• 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.

[foolnotion]

note that the switch to qt6 is likely to affect theming for a lot of people. qt support outside kde is wonky at best. I use swaywm and qbittorret-4.6.0 from your branch does not obey any theme.

[paveloom]

@foolnotion I'm not aware of possible theming issues on Sway. That said, I've added qtwayland as a dependency in the latest force push. I would suggest trying again. Make sure you have the Qt theme set, though. Here's what I use on NixOS:

qt = {
  enable = true;
  platformTheme = "gnome";
  style = "adwaita";
};

[foolnotion]

@foolnotion I'm not aware of possible theming issues on Sway. That said, I've added qtwayland as a dependency in the latest force push. I would suggest trying again. Make sure you have the Qt theme set, though. Here's what I use on NixOS:

qt = {
  enable = true;
  platformTheme = "gnome";
  style = "adwaita";
};

I have something similar, I'd suggest worry about this after it gets merged. Just be aware a lot of people have issues with theming e.g. #260696

[peterhoeg]

The issue is with KDE users as the current qt5 based Plasma doesn't have any Qt6 themes. There really is no way around that until Plasma 6 lands. The nicer way to do this is to build both qt5 and qt6 versions of this and have the end-user choose. This is what we do for a number of other applications that support both 5 and 6.

[paveloom]

All right, both Qt versions are built now.

[Tungsten842]

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

1 package marked as broken and skipped:

• springLobby

12 packages built:

• btfs
• deluge (deluge-2_x ,deluge-gtk)
• deluge.dist (deluge-2_x.dist)
• deluged
• deluged.dist
• libtorrent-rasterbar (libtorrent-rasterbar-2_0_x)
• libtorrent-rasterbar.dev (libtorrent-rasterbar-2_0_x.dev)
• libtorrent-rasterbar.python (libtorrent-rasterbar-2_0_x.python ,python311Packages.libtorrent-rasterbar ,python311Packages.libtorrent-rasterbar.dev ,python311Packages.libtorrent-rasterbar.python)
• python310Packages.libtorrent-rasterbar (python310Packages.libtorrent-rasterbar.dev ,python310Packages.libtorrent-rasterbar.python)
• qbittorrent
• qbittorrent-nox
• qbittorrent-qt5

[Tungsten842]

It seems that it doesn't build on darwin:
https://logs.ofborg.org/?attempt_id=1ebf546e-0768-4d0f-a76b-1c4cd015b2cf&key=nixos%2Fnixpkgs.263111

[paveloom]

It builds, just the install script needs a little bit of tweaking. I can't really debug it since I don't have a suitable Darwin machine.

[Tungsten842]

It builds, just the install script needs a little bit of tweaking. I can't really debug it since I don't have a suitable Darwin machine.

In this case I think that you should either mark the darwin platform as broken, or simply remove the darwin support for now, until someone figures out how to fix it.

[paveloom]

Marked as broken on Darwin.

[Tungsten842]

Tested and working as expected

[mjm]

diff --git a/pkgs/applications/networking/p2p/qbittorrent/default.nix b/pkgs/applications/networking/p2p/qbittorrent/default.nix
index 27330a97c1...8842ba0aed 100644
--- a/pkgs/applications/networking/p2p/qbittorrent/default.nix
+++ b/pkgs/applications/networking/p2p/qbittorrent/default.nix
@@ -75,7 +75,7 @@
 
   postInstall = lib.optionalString stdenv.isDarwin ''
     mkdir -p $out/{Applications,bin}
-    cp -R src/${pname}.app $out/Applications
+    cp -R ${pname}.app $out/Applications
     makeWrapper $out/{Applications/${pname}.app/Contents/MacOS,bin}/${pname}
   '';
 

This change makes it work for me on aarch64-darwin.

[paveloom]

@mjm Nice! And the nox one is called qbittorrent-nox.app?

[mjm]

Indeed it is! I've been able to build all 3 variants (qt6, qt5, nox) with your latest changes.

[peterhoeg]

One last thing - you're using both lib.optional and lib.optionals in there - the former is not recommended due to its poor error handling, so the better way is something like this:

1. add inherit (lib) optionals; to the top let bit
2. reference just optionals (without the lib) everywhere you are currently using lib.optionals
3. change existing lib.optional to optionals and make the 2nd parameter a list

[peterhoeg]

You can of course use the full lib.optionals but this way it becomes easier for an erroneous lib.optional to sneak its way in.

[paveloom]

Done with fully qualified calls.

[peterhoeg]

merge conflicts

[peterhoeg]

Very last thing (I promise!) - please squash the qbittorrent commits into one, so that this PR contains just 2: 1 regarding libtorrent-rasterbar and the 1 concerning qbittorrent.

We cannot do selective squashing through GH.

[paveloom]

Why squash all into one? This will make the diff unreadable.

[peterhoeg]

As an example. the “switch to cmake” commit doesn’t make any sense on its own. Nobody is going to say “we need to revert *this* commit but leave everything else in”. So the changes related to qbittorrent should be one commit and then libtorrent another. The diff will stay just as readable as it is today.

[paveloom]

I would expect all relevant commits to be reverted as well, if needed. They are related under one merge anyway, so one would just revert the merge. All of the following commits from future merges will depend on changes done here, so it's not like one could just revert one commit in general (especially the CMake one that changes the build system in use).

Per-commit diffs are much easier to read and understand when their summary states one logical change. They are also useful when you actually encountered an issue and want to find its source (be it by reading the summaries of commits or by using git bisect). All of the commits here can be checked out, and qBittorrent can be compiled from all of them (except for Darwin systems, but that's because I don't have a suitable machine at the moment).

That said, I don't care too much about protecting these opinions, so I squashed the commits anyway.

[peterhoeg]

so it’s not like one could just revert one commit in general (especially the CMake one that changes the build system in use).

And that’s exactly the point - the individual commits all touching the same file for what is essentially just a version bump are just rolled into a single commit.

Per-commit diffs are much easier to read and understand when their summary states one logical change. They are also useful when you actually encountered an issue and want to find its source (be it by reading the summaries of commits or by using git bisect).

Again, there is nothing special in this - it’s a very straight-forward version bump so the individual commits are not interesting. If you feel that anything in the code could benefit from additional information, adding a few comments would be very helpful.

That said, I don’t care too much about protecting these opinions, so I squashed the commits anyway.

Super. :+1: