Skip to content
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

Use installer resources for DSM6, update TCL package #4562

Closed
wants to merge 7 commits into from

Conversation

hgy59
Copy link
Contributor

@hgy59 hgy59 commented Apr 15, 2021

Motivation: Another Framework cleanup and a package update for TCL
Linked issues: #4524, #4539

Checklist

  • Build rule all-supported completed successfully
  • Package upgrade completed successfully
  • New installation of package completed successfully

Remarks

  1. Revert SPK_LINKS Makefile variable and use SPK_USR_LOCAL_LINKS for DSM6 and DSM5.
  2. Update TCL and make use of SPK_LINKS obsolete, and add 64-bit support for amd64 archs
  3. Adjust remaining packages to use SPK_USR_LOCAL_LINKS instead of SPK_LINKS
  4. Redesign DSM6 installer to use resource definitions for Links, FwPorts, SHARES similar to DSM7
  5. Only keep creation of /usr/local/{package} link to avoid breaking already installed packages that lean on this

References

@hgy59 hgy59 changed the title Use installer resources for DSM6 Use installer resources for DSM6, update TCL package Apr 15, 2021
@hgy59 hgy59 added dsm 6 framework update request to update existing package labels Apr 15, 2021
@@ -182,6 +170,13 @@ ifneq ($(strip $(SERVICE_WIZARD_SHARE)),)
'."data-share" = {"shares": [{"name": $$share, "permission":{"rw":[$$user]}} ] }' $@ 1<>$@
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something interesting to note:

Since 7.0-41201, package center will create a symlink under /var/packages/[package_id]/shares/ named by share folder pointing to share folder path.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have to test shared folders with DSM6 anyway. I will look whether dsm6 resource worker will create such links too.

Copy link
Member

@publicarray publicarray left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@hgy59
Copy link
Contributor Author

hgy59 commented Apr 17, 2021

LGTM

I have to validate the SHARE_PATH (i.e. SERVICE_WIZARD_SHARE) still works for DSM 6

@hgy59
Copy link
Contributor Author

hgy59 commented Apr 17, 2021

@publicarray I tend to redesign the shared folder handling for DSM6.

I did not succeed to mix full path (i.e /volume1/downloads) and shared folder name (i.e. downloads) with one installer.
Finally the breaking fact is that it is not possible to force back variables (i.e. wizard_shared_folder_name) into the enviroment so that the system uses it for the "data-share" resource definition.
I also tried to patch the conf/resource file with the share_name extracted from full path. The resource file exists at the final location in the postinst step and the patch was sucessful, but the installation failed and the name of the shared folder was not taken from the updated resource file (it still took the name from the wizard variable when I defined both).
I even tried to use /usr/syno/sbin/synopkghelper update ${package} data-share ignoring that for data-share resources the documentation says it is not updatable (only port-configs seem to be updatable). Unsurprising error Failed to update data-share of demoservice [0xD900 resource_api.cpp:331].
The synology dev docu says that the data-share resource worker activates resources at enable step but this seems to be wrong (just another example that this docu is worth nothing).

The shared folder processing in the sc framework (i.e. with /volumeX/share_name) was good for DSM<=5.
With the sc framework we are suffering again on the incomplete implementation of the DSM6 development guide.

When creating a folder under /volumeX we do not really get a DSM shared folder (this might be the cause for a lot of permission issues discussed here). Such folders are not displayed under shared folders and are deletable by the root user - shared folders cannot be deleted by root, but only within "Control Panel > Shared Folders".

IMHO we must migrate to "data-share" resource definitions for DSM6,
This includes the requirement to adjust all wizards for packages that use shared folders.

The benefit would be real shared folders. And we could define default access for sc-download group in the resource file too.

Just another evening spent on digging into the swamp of DSM. 😩

@publicarray
Copy link
Member

publicarray commented Apr 18, 2021

@hgy59 I've pushed transmission package form dsm7-packages (feel free to remove it again). But I used this as my test package since I already did most of the work on it. In my tests it can install on a fresh dsm6 install (this branch) and can upgrade from an older version (master branch) of the transmission package on the dsm6. (both with the default settings/shared folder). I added a commit to set the wizard_download_dir variable when upgrading for the worker to succeed (wizard).

@publicarray

This comment has been minimized.

"desc": "Download shared folder (using the volume chosen above)",
"subitems": [
{
"key": "wizard_download_dir",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aaah, that is the trick, to use separate wizard variables for the volume and the share name!

but this should work in the demoservice too.

Copy link
Member

@publicarray publicarray Apr 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only the folder name is used in the worker (not the full path), maybe we need to use basename before passing it on to jq --arg share [..]. I forgot about that too sorry.

Edit: Yes it works with the demoservice, I just messed up earlier (I still passed on the full path)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Example from documentation:

"data-share": {
    "shares": [{
        "name": "music",
        "permission": {
             "ro": ["AudioStation"]
         }
    }]
}

Copy link
Member

@publicarray publicarray Apr 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hgy59 whichever way we go, (new wizards or patch to remove volumeX/ before passing the variable on) please remember to remove the if:

include ../../mk/spksrc.common.mk
ifneq ($(call version_ge, ${TCVERSION}, 7.0),1)
# SERVICE_WIZARD_SHARE is not supported for DSM7 yet
SERVICE_WIZARD_SHARE = wizard_download_dir
endif

Copy link
Contributor

@ymartin59 ymartin59 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not appreciate such long PR with unrelated content.
Please split it into three distinct branches: links, tcl and transmission

@hgy59
Copy link
Contributor Author

hgy59 commented Apr 25, 2021

so we gonna split this PR

@hgy59 hgy59 marked this pull request as draft April 25, 2021 10:09
This was referenced Apr 25, 2021
@hgy59
Copy link
Contributor Author

hgy59 commented May 14, 2021

@publicarray I would like to close this PR, as my work was merged with #4577 and #4578.
Just want to ask you, whether nothing is lost about your work for shared folders on transmission package here.

@publicarray
Copy link
Member

publicarray commented May 22, 2021

Thanks @hgy59 I'll hopefully get to it early next month. Just busy with work ATM

(A quick rebase would show what is left over)

hgy59 and others added 7 commits May 22, 2021 09:06
- SPK_LINKS is not supported anymore, use SPK_USR_LOCAL_LINKS and/or SPK_COMMANDS
- adjust all packages with former SPK_LINKS (beets, borgbackup, rdiff-backup)
- resource workers for ports/links/shares are supported from DSM>=6.0
- keep the /usr/local/{package} link in DSM 6 for now, as not all dependent packages are redesigned
yes, here we can

Co-authored-by: Sebastian Schmidt <[email protected]>
set using SERVICE_WIZARD_SHARE

only disadvantage is that this does not support sub-folders currently

[Transmission] fix empty volume, consistency with naming
* Remove busybox dep

* transmission: remove unnecessary GROUP setting from service-setup.sh

* transmission: set GROUP for backwards compatibility

* Undo SPK_REV/CHANGELOG

* Adjust wizard text

* Remove now superfluous busybox configs
@publicarray
Copy link
Member

The transmission stuff is in #4719 closing this one.

@hgy59 hgy59 deleted the use_dsm_resources branch August 28, 2021 11:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dsm 6 framework update request to update existing package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants