qbittorrent: add python as a runtime dependency for tracker search #59293
Conversation
|
@GrahamcOfBorg eval |
|
@GrahamcOfBorg eval |
|
Could call for the maintainer, if we do the patching. |
Do you mean I should add myself as a maintainer? If you want to avoid patching upstream code we could use wrapProgram instead. It would wrap qbittorrent with a shell script, but it would probably be easier to maintain. There is a similar discussion in #57590. |
| @@ -25,6 +27,17 @@ stdenv.mkDerivation rec { | |||
| buildInputs = [ boost libtorrentRasterbar qtbase qttools qtsvg ] | |||
| ++ optional guiSupport dbus; # D(esktop)-Bus depends on GUI support | |||
|
|
|||
| patchPhase = '' | |||
There was a problem hiding this comment.
I am not really a fan of this approach.
Do we really need to keep the patch to patch upstream code to delete their tests just to add Python3 as a runtime dep?
There was a problem hiding this comment.
Also, if suddenly user would have python3 installed - does he still acquires what is set - no search in qbittorrent?
There was a problem hiding this comment.
The goal of this patch was to avoid reading python from PATH at runtime so this would only ever use the version of python specified at build time.
There was a problem hiding this comment.
This was needed to be in the comments. All special cases code should be noted.
Indeed, but in the wrapper case, the python would also be preserved.
So there is a question of maybe leaving runtime dep to the user to install.
This is a Nix package manager limitation.
| @@ -3,9 +3,11 @@ | |||
| , debugSupport ? false # Debugging | |||
| , guiSupport ? true, dbus ? null # GUI (disable to run headless) | |||
| , webuiSupport ? true # WebUI | |||
| , searchSupport ? true, python ? null # Search | |||
There was a problem hiding this comment.
Do not see the point in this option.
WebUI support - ok.
GUI/headless - ok.
Does anyone really would want the torrent without search support particularly.
There was a problem hiding this comment.
I don't see why anyone would want to disable it, but the INSTALL file in qbittorrent says that python is optional so I decided to add an option.
|
@MetaDark You'd provided more information about the problem you have. And it is not about regular search, it is about search on torrent trackers. |
Regular search is referred to as filtering in qbittorrent, but I could rename the option to searchEngineSupport to make it less ambiguous. |
|
In NixOS distribution it is more logical to name things not in terms of qbittorrent, but in terms of user perspective who installs the software. BTW, if you after adding python - just add wrapper. If you want to have switch for these search on trackers - it seems reasonable only if that support can be disabled at build-time through build keys. So I would say - if you don't need it - don't bother, there is no reason to add and support a feature if it had no user with a reason asking for it. |
kira-bruneau
changed the title
|
@MetaDark You also can use TODO (mark point for the future work), NOTE, FIXME (code that is for the time being and needs to be rewritten), HACK (hacking, working around the upstream code). These are the most standardized tags that everyone understands. I do also supply the date of the comment in ISO format: # NOTE: 2019-04-12: This is because ...Making a status and date in front of eyes makes it easier even for me myself to review my own code. |
|
@GrahamcOfBorg build qbittorrent |
Motivation for this change
This change wraps qbittorrent with python as a run-time dependency. This ensures that python is included in the resulting closure for tracker search support.
Things done
sandboxinnix.confon non-NixOS)nix-shell -p nix-review --run "nix-review wip"./result/bin/)nix path-info -Sbefore and after)