-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
ruTorrent: Update to 5.2.10 #6810
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
ruTorrent: Update to 5.2.10 #6810
Conversation
|
hey @smaarn, I've gone ahead and updated ruTorrent in a number of areas. Most importantly we are changing the PHP back-end to support newer DSM versions. Let me know if you have time for a review. |
smaarn
left a comment
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.
There are a few changes which seem to be some kind of convention changes (this thing migrating \`` to $(` which is beyond my expertise I must confess.
I only have a few comments / questions (would need to test it, will try to see if I can do that by EOD)
|
@mreid-tt please omit the php version in the profile description for all php profiles. |
|
regarding libmediainfo: If you really want to use 25.10 for newer compilers, you must update the mediainfo package too and drop support for DSM < 7.x. I recently updated the mediainfo package and have chosen to use the latest version that builds for DSM 6. |
Yes, both |
|
Hey @smaarn, I came across an interesting discussion here: Novik/ruTorrent#2983 about the long-standing issue where newer rTorrent versions break the ratio plugin. I downloaded the latest rTorrent, applied one of the suggested patches plus one of my own, and it seems to build and install correctly. Here’s the current run if you’d like to test it: I’m not planning to push this to the PR yet, since the latest rTorrent release has an open bug (rakshasa/rtorrent#1650). I’ll wait for the next version and try again once it’s out. My limited tests (on DSM 7.1) so far show:
—but that’s as far as I’ve tested for now. |
This is a missunderstanding. OK, now I can see that spk/rutorrent/Makefile depends on cross/mediainfo If rutorrent only needs mediainfo at runtime, we should remove the mediainfo dependency completely and add SPK_DEPENDS = mediainfo instead. If rutorrent requires mediainfo at build time, I guess it can depend on cross/libmediainfo instead of cross/mediainfo. The least required change is to modify spk/mediainfo/Makefile to depend on either cross/mediainfo-25.04 OR on cross/mediainfo-latest but not on a gcc version dependent cross package. Otherwise we will build packages for DSM 6 with invalid version number. further thoughts: /CC @th0ma7 @publicarray |
Hmm, the reason I split both mediainfo and libmediainfo is because they have different releases upstream and I assumed that they needed to have matching versions. i.e. We shouldn't have a cross/mediainfo that may depend on cross/libmediainfo-latest when it really needs cross/libmediainfo-25.04 as I assumed it needed a matching version to build correctly.
I'm not exactly sure what type of dependency it needs but I assume it has something to do with the mediainfo plugin (https://github.com/Novik/ruTorrent/wiki/PluginMediainfo). This is not a package I'm intimate with and it has many layers so I was trying to make minimal changes.
If I"m understanding you correctly, you are concerned about the spk/mediainfo package possibly building from the cross/mediainfo based on the GCC version which could result in the 25.04 version building on a 25.10 cross if it's a newer GCC? If so that is a fair call and we would probably need to change spk/mediainfo to just depend on latest and restrict the spk/mediainfo to DSM 7 only. If I'm not understanding you correctly please let me know. |
|
looking deeper: rutorrent needs the mediainfo binary for the mediainfo plugin. The path to mediainfo is defined in config.php and the code to patch this file is in service-setup.sh. So it is easy to remove mediainfo from the rutorrent package and depend on the mediainfo package and adjust the path. Analysis of service-setup.sh showed another issue. ATM the config.php file is updated on package install but not on upgrade, so this must be fixed for upgrades too, Additionally I propose to remove the DSM 6 to DSM 7 migration in this release and, for the rare case that someone still tries to migrate from DSM 6, to display an error that to update rutorrent a manuall update with package version 5.1.12-18 has to be done first (and for some time we must not remove rutorrent 5.1.12-18 in the repo). Another proposal is to use pgrep of cross/procps-ng to get rid of the busybox stuff (dependency and configuration for build, and initialization in service-setup). |
Okay so we can switch the package to have mediainfo as a package dependency rather than build dependency. I'm okay with that. @smaarn any objections?
Good catch. I've tested on a DSM 7.3 box and you're right that the existing path with pick up the OS version as follows: Whereas a specific path to the installed expected version would report this:
Agreed, as this could create a potential edge-case where we would need to re-evaluate this if the user upgrades their DSM version and they then need a package upgrade but the package dependencies may no longer match what's in the config.php. Will think on this some more.
I thought I removed some of it but will look again.
I'm not familiar with this so will need to do some research. |
only this is needed:
|
No objection. I would even recommend it. Personally I think it's easier to maintain this way since it makes maintenance decorrelated.
Actually those are defined by the below kind of lines: If you wish to force the PHP binary path you can use those sections but I'm not sure it's even relevant (last time I checked it didn't cause an issue). |
|
Hey @hgy59 @smaarn, I think I’ve made some solid progress on the update. Now that the latest rTorrent/libtorrent (0.16.4) releases have landed, I’ve integrated them into the package. I also removed the version bumps and the mediainfo split, switching to the existing external package instead. As suggested, I’ve dropped BusyBox and refactored a good portion of the service-setup script to address DSM 7 concerns, removing the older DSM 6 logic entirely. I’ve tested the build successfully on DSM 7.1 and 7.2, but DSM 7.3 is proving problematic. There’s very little in the logs to point to the issue. The I’ve kicked off a workflow run here so you can try the DSM 7.2+ build on DSM 7.3 if you have a moment: If we can’t resolve the DSM 7.3 issues, I’ll exclude it from the supported versions for now. At this point, the only remaining cleanup I can see is the installation wizard. Let me know if your reviews highlight anything else. |
th0ma7
left a comment
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.
Really nice work, LGTM!
|
I get the below error when starting the UI, not sure of the impact though: |
smaarn
left a comment
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.
No strong opinion on my side really, apart from the complexity of the external software path handling which seem overly complex to me but 🤷
I'm only wondering about the warning of the UI
When opening the console it seems that it's related to the |
You have it working successfully on DSM 7.3?!? I couldn't get it to work at all. Do you have any of the errors I noted above? EDIT: Because I couldn't get DSM 7.3 to work I included a commit to limit its installation to DSM 7.2. EDIT: Merging as-is and we can re-visit DSM 7.3 support at a later date. |


Description
This PR contains the following:
Fixes #
Checklist
all-supportedcompleted successfullyType of change