rutorrent: DSM7 support + fix torrent creation wizard #5455
Conversation
|
@smaarn this is awesome, thnx for your assistance on this, really much appreciated. A few things for considerations:
Let me know if/how I can provide assistance. |
|
@th0ma7 will be having a look at the deluge folder conventions to see if I can match it easily with the defaults. Regarding your "2x" services startup I'm not sure what you mean but will be having a look at how it's done in deluge. The rutorrent 4.0 migration is definitely something we should be doing asap but I will be, for now, focusing on having the DSM 7 installation working and operational before diving into 4.0 migration (since their documentation on what was "changed" is pretty scarce I would tend to favor a conservative approach). |
I revisited this and you're right, there isn't "two" daemons but rather:
So there really is only one daemon and it's call could be integrated into the BTW, if you want to play with the screen session, first identify it: Then attach to it: To get help once connected use |
smaarn
force-pushed
the
rutorrent/upgrade-dsm-7
branch
from
46058a3 to
5e4f359
Compare
smaarn
force-pushed
the
rutorrent/upgrade-dsm-7
branch
from
881b1d1 to
b8218c2
Compare
|
Package tested manually under DSM 7: both install and upgrade works fine. The open point is about DSM 6 not being broken (don't have a DSM 6 around so ... :/ ). Don't see why it would but then I changed a few things regarding the upgrade process which may have some unforeseen impacts. Will be following up on this in the coming days. |
|
This is awesome work! I still have a dsm6 so I'll test it over the coming days. Again, great work and long overdue! |
smaarn
force-pushed
the
rutorrent/upgrade-dsm-7
branch
from
cdb351e to
a2f4ece
Compare
smaarn
changed the title
|
I propose to remove the configuration of the port range from the wizards. |
| { | ||
| "desc": "If the specified share does not exist, it will be created. You can use an existing share by specifying the name of the folder." | ||
| }, | ||
| { | ||
| "type": "textfield", | ||
| "desc": "Watch a directory for torrent files and add them to ruTorrent. Leave empty to disable", | ||
| "subitems": [ | ||
| { | ||
| "key": "wizard_watch_dir", | ||
| "desc": "Watch directory", | ||
| "validator": { | ||
| "allowBlank": true, | ||
| "regex": { | ||
| "expr": "/^\\\/volume[0-9]+\\\//", | ||
| "errorText": "Path should begin with /volume?/ with ? the number of the volume" | ||
| } | ||
| } | ||
| } | ||
| ] | ||
| }, |
There was a problem hiding this comment.
IMHO this share must be separated to volume and share name as above for the download location
OR you use the same volume as above (we did not test whether the creation of multiple shared folders on different volumes for the same package is supported by DSM).
OR you create the watch folder as a sub directory of the download shared folder, then you have to adjust at least the validation regex.
There was a problem hiding this comment.
I would add, why not simply follow the exact same directory tree as deluge and enforce that by default such as:
downloads/
├── complete
├── incomplete
└── watch
There's a dsm6-7 "compatible" method to create the folders from deluge service-setup.sh script:
# Create download directories
install -m 0775 -o ${EFF_USER} -g ${GROUP} -d "${incomplete_folder}"
install -m 0775 -o ${EFF_USER} -g ${GROUP} -d "${complete_folder}"
install -m 0775 -o ${EFF_USER} -g ${GROUP} -d "${watch_folder}"
There was a problem hiding this comment.
We should use a different watch folder for each package, as we do not want to download a watched file by all watching applications (and it will depend on whether an application removes the files in the watch folder when download starts vs when the download is completed).
There was a problem hiding this comment.
Good point, maybe worth defaulting it to watch-<appname> then? I could make the changes to deluge while i have an opened pr.
There was a problem hiding this comment.
Actually I would propose to have a combination of
- A checkbox allowing for usage of a watch directory (disabled by default)
- An input component defaulted to the "watch" subdirectory
WDYT ?
I am actually not sure that rutorrent supports the complete / incomplete logics.
There was a problem hiding this comment.
Sounds good to me.
As for the complete vs incomplete directories, if I recall you have to set the default directory to incomplete and use the move upon completion feature so it, well, move the resulting torrent to the complete folder once over.
One thing i noticed is the incredible performance difference between deluge vs rutorrent. Deluge is numerous time more performant ... There may be config options to fine tune rutorrent...
Traveling this week so my availability may vary.
There was a problem hiding this comment.
Sorry for the delays here.
First things first: my idea of having a checkbox is actually not feasible it seems with regards to the current Javascript framework which is used to handle the installer logics (short version: can't listen for a checkbox change to actually change visibility of a component, I can only do that on page changes)
I'm not (at all) an expert in rtorrent configuration, I may be having a look at this but it may take some time.
And for the performance part I must confess I don't use rutorrent (apart for testing, I'm more of a transmission user) and only had a crack at it to unblock some python logics so I can't really argue with you here :)
Will try having a look today.
There was a problem hiding this comment.
@smaarn, is this concern regarding the UI wizard logic now resolved?
There was a problem hiding this comment.
@mreid-tt no it's not. The wizard option is actually a dead-end and the actual rutorrent options need being tested.
I must confess I didn't consider investing more time in "normalizing" those options. TBH as a Transmission user I found it puzzling on my last upgrade to see the paths change so not sure it's something we want to force, is it ?
I mean, for an initial installation, it's arguable, but I'd say that for an existing installation it can be seen as a little bit "invasive" for a policy, is it not ?
There was a problem hiding this comment.
First things first: my idea of having a checkbox is actually not feasible it seems with regards to the current Javascript framework which is used to handle the installer logics (short version: can't listen for a checkbox change to actually change visibility of a component, I can only do that on page changes)
@smaarn, as a side note, I was working on a more robust uninstall UI wizard for ownCloud (uninstall_uifile.sh) and after going down a trial-and-error rabbit-hole, I can confirm the issue with checkboxes and component visibility. I did get it to partially listen for a checkbox to make another item visible using a validation function but there were two major issues:
- The actual checkbox itself when clicked didn't do anything
- The checkbox label did trigger the change but only after clicking elsewhere (remove focus)
Either way, this was not an ideal situation. I did however find a use-case to apply a validation function to a text field and based on the input, enable checkboxes (or other fields) as required. From a UX perspective it works because the user would want to click one of the empty checkboxes after entering a path and this action would make the text field loose focus – immediately enabling the checkboxes before the click is registered. I remembered this when I wrapped up and thought I'd share my findings.
I would even add that altering the file configuration wouldn't make it work natively (meaning we would need to ensure that upon file configuration changes the firewall settings are updated... which could be a terrible thing to implement and do). Doing that would require actually adding a rutorrent documentation section to be fair. |
|
@smaarn upgrading from DSM6 failed. Overall the logs looks ok rutorrent-DSM6-upgrade.txt like it failed silently? Looking further, it actually fails at fixing permissions somewhere ( EDIT: Although excellent news, it works really well with a fresh install! woot! so just needing to figure out what's wrong with the update.
EDIT (additional findings) -
|
|
Recap, success so far:
issues found so far:
|
Need to fix the handling of former wizard variables.
... Need to check.
Can change the default but I believe this was already the case before.
It's indeed randomized within the specified range. Not too sure about the range size TBH. |
smaarn
force-pushed
the
rutorrent/upgrade-dsm-7
branch
from
67708c0 to
b7360fe
Compare
|
Ok so some news:
With regards to the proposed folder structures I actually wonder if it's really worth having a watch directory enforced by default and worrying about concurrent application usages. |
spk/rutorrent/Makefile
Outdated
| ENV += BUSYBOX_CONFIG="$(BUSYBOX_CONFIG)" | ||
|
|
||
| # some dependents require C++11 support | ||
| UNSUPPORTED_ARCHS = $(ARMv5_ARCHS) $(OLD_PPC_ARCHS) | ||
| # unsupported with cross/rtorrent | ||
| UNSUPPORTED_ARCHS += $(ARMv7L_ARCHS) | ||
|
|
||
| SYSTEM_GROUP = http | ||
|
|
||
| ICON_DIR = $(STAGING_DIR)/$(DSM_UI_DIR)/images |
There was a problem hiding this comment.
This is a hack to force the generation of icons. A rework of the framework could be useful to force them (otherwise DSM 7 specific resource workers wouldn't be using available icons)
There was a problem hiding this comment.
I'm not sure I follow, what's missing exactly?
There was a problem hiding this comment.
What's missing is that without this trick the icons are not automatically generated. Will rehave a look because framework has evolved lately but it seems icons were only generated in some very specific context.
There was a problem hiding this comment.
looking at spksrc.icon.mk, icons are created when DSM_UI_DIR and SPK_ICON and SERVICE_PORT are defined and NO_SERVICE_SHORTCUT is not defined.
I suppose this will be fixed by adding to Makefile:
SERVICE_PORT = 8050
There was a problem hiding this comment.
Won't this be causing issues with the interactions with the way you can "open" the page ? (Checking)
There was a problem hiding this comment.
And probably you can use the SERVICE_PORT variable when patching the scgi_port in config.php at installation time.
There was a problem hiding this comment.
Won't this be causing issues with the interactions with the way you can "open" the page ? (Checking)
I suppose it does not. It could change the generation of the app/config file, but you have a custom app/config file that is in src/app/config and is installed in rutorrent_extra_install. So you have to check that this file is still the same when adding SERVICE_PORT definition.
There was a problem hiding this comment.
@smaarn, is this concern regarding the package icon generation now resolved?
smaarn
force-pushed
the
rutorrent/upgrade-dsm-7
branch
from
b7360fe to
5dac8d4
Compare
|
(flagging as draft since I did a few rework on the installation / upgrade procedure, still need to retest everything) |
|
@hgy59 : removed the port customization feature There are two open points:
It turns out that
I will try to find some time to work on the folders' separation logic and 4.0 upgrade within the coming weeks. As is, though (once the DSM 6 update scenario is validated), I believe it should be worth merging it. WDYT ? |
smaarn
force-pushed
the
rutorrent/upgrade-dsm-7
branch
from
055bce9 to
1c26149
Compare
|
@smaarn, new build seems to resolve all remaining issues in DSM6. no errors in the interface when launched...
Installation log also looks clean -- https://pastebin.com/Ev0j4zUi. |
|
I also did a test download Test Torrent and confirmed that the application does download the torrent successfully...
Resulting in these in the downloads folder...
|
|
@smaarn, an upgrade from the current v3.10-10 on DSM6 is reporting as failed. The installation log seems to be fine though... https://pastebin.com/cJSQGY27.
EDIT: I'm seeing this in the The failed data-share acquisition may be the cause... because in a fresh install it looks like this: EDIT: This may have come up before with the framework in #4766. Perhaps this can help. |
|
@smaarn, your latest build was a success for upgrades from the current v3.10-10 on DSM6! See video... rutorrent_x64-6.1_3.10-13.spk-upgrade.mp4 |
|
Thanks a lot for the help in testing ! |
|
4.0.2 upgrade comming in a follow up MR (here: #5617 ) but with my limited availabilities didn't want to put it in the critical path. Here we have now a DSM 7 ready version to publish |
|
@smaarn was this published? I don't see it online. Let me know if you need assistance. |
@smaarn I'll take care of it.
Yes, @publicarray made an awesome documentation online.
So now on my way to publish this PR. EDIT: build + publishing online in progress https://github.com/th0ma7/spksrc/actions/runs/4420411967 EDIT2: Now published |
hgy59
added
status/published
…5455) * Add DSM 7 support * Migrated to PHP 7.4 * Fix torrent creation wizard (SynoCommunity#5288) * Add explicit dependency on WebStation * Add firewall service port declarations
Description
Having rutorrent DSM7 ready + fix cryptic php error when trying to create torrent file via wizard
Fixes #5288
Checklist
all-supportedcompleted successfullyType of change