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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
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.
Now the package does not change the upstream code.
Well, now recent python3
would be included in the closure, because qbittorent
has by default Search Engines UI (search on trackers) feature.
@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
sandbox
innix.conf
on non-NixOS)nix-shell -p nix-review --run "nix-review wip"
./result/bin/
)nix path-info -S
before and after)