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

ownCloud: Update to v10.13.2 #5912

Merged
merged 29 commits into from
Nov 2, 2023
Merged

Conversation

mreid-tt
Copy link
Contributor

@mreid-tt mreid-tt commented Oct 15, 2023

Description

This PR contains the following:

  1. Update ownCloud to version 10.13.2
  2. New backup and restore functions
  3. Migrate to new shared folder handling
  4. Update wizards to simplify maintenance (using mustache)
  5. Remove synthetic service daemon (using webservice)
  6. Fix for Add mustache to dev env #5819 to remove default uninstall template if shell script present

Fixes #

Checklist

  • Build rule all-supported completed successfully
  • New installation of package completed successfully
  • Package upgrade completed successfully (Manually install the package again)
  • Package functionality was tested
  • Any needed documentation is updated/created

Type of change

  • Bug fix
  • Package update
  • Includes small framework changes

@mreid-tt mreid-tt self-assigned this Oct 15, 2023
@mreid-tt mreid-tt mentioned this pull request Oct 15, 2023
3 tasks
@mreid-tt mreid-tt requested a review from smaarn October 15, 2023 03:26
Copy link
Contributor

@smaarn smaarn left a comment

Choose a reason for hiding this comment

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

I think the only issue here with the wizards are the fact that they are using ".sh.yml" suffixes when it should only be ".yml"
Apologies for not updating the docs

spk/owncloud/src/wizard_templates/install_uifile.sh.yml Outdated Show resolved Hide resolved
@mreid-tt
Copy link
Contributor Author

I think the only issue here with the wizards are the fact that they are using ".sh.yml" suffixes when it should only be ".yml" Apologies for not updating the docs

Thanks for the explanation, was simple to resolve. Appreciate your PR to add this as it does greatly simplify multilingual wizards. Look forward to the updated docs.

@hgy59
Copy link
Contributor

hgy59 commented Oct 15, 2023

@mreid-tt any feedback on how to migrate to the new shared folder handling is appreciated. I didn't have time to dig into it...

@mreid-tt
Copy link
Contributor Author

@mreid-tt any feedback on how to migrate to the new shared folder handling is appreciated. I didn't have time to dig into it...

The new shared folder handling seems to work well. Tested this package with a /volume2 installation on both DSM 7 and DSM 6 without any issues.

@mreid-tt mreid-tt requested a review from hgy59 October 15, 2023 16:53
@mreid-tt
Copy link
Contributor Author

@smaarn, I don't know if your PR catered for excluding the default uninstall_uifile if the wizard template generates its own file uninstall_uifile.sh. Currently the built package contains both. While it doesn't appear to affect uninstalls it would be great if the default uninstall wizard were not present.

@hgy59 I know you are a master at the framework and may have a quick solution to the above (to amend #5819). Any thoughts come to mind?

@hgy59
Copy link
Contributor

hgy59 commented Oct 15, 2023

The new shared folder handling seems to work well. Tested this package with a /volume2 installation on both DSM 7 and DSM 6 without any issues.

Found an issue when migrating #5213 to new shared folders:
On DSM 7 a package update from the old to the new shares works as expected, but on DSM 6 it fails.

In mk/spksrc.service.installer.functions in the function initialize_variables () the command abspath must be replaced by realpath since abspath is not a valid command on DSM.

        if [ -z "${SHARE_NAME}" ]; then
            SHARE_NAME=$(echo $(realpath ${SHARE_PATH}) | awk -F/ '{print $3}')
        fi

But the real problem under DSM 6 is, that the shared folder cannot be created from the resource file on package update when the wizard variable with the share name is not defined.

I fear that only on DSM 7, where the shared folders are (always) created by resource files, a package update without an upgrade wizard works.
For DSM 6 and migration from manually created share to resource worker by package upgrade, we need one of

  • an upgrade wizard to reinitialize the wizard variable for the share name from etc/installer-variables of the installed package
  • patch the resource file to replace the placeholder (wizard-variable) with the share name taken from etc/installer-variables of the installed package
  • initialize the wizard-variable from etc/installer-variables of the installed package without an upgarde wizard

I will have to investigate, whether it can be fixed by initialization of the wizard-variable or a patch of the resource file (or a wizard) is required.

@mreid-tt
Copy link
Contributor Author

mreid-tt commented Oct 16, 2023

@smaarn, I don't know if your PR catered for excluding the default uninstall_uifile if the wizard template generates its own file uninstall_uifile.sh. Currently the built package contains both. While it doesn't appear to affect uninstalls it would be great if the default uninstall wizard were not present.

I was looking at some of the code again and I see this section:

spksrc/mk/spksrc.spk.mk

Lines 388 to 391 in ecce16f

ifeq ($(call version_ge, ${TCVERSION}, 7.0),1)
@$(MSG) "Create default DSM7 uninstall wizard"
@mkdir -p $(DSM_WIZARDS_DIR)
@find $(SPKSRC_MK)wizard -maxdepth 1 -type f -and \( -name "uninstall_uifile" -or -name "uninstall_uifile_???" \) -print -exec cp -f {} $(DSM_WIZARDS_DIR) \;

This seems to just take the stock uninstall_uifile and copy it into the DSM_WIZARDS_DIR.

Then we have this section:

spksrc/mk/spksrc.spk.mk

Lines 396 to 398 in ecce16f

ifneq ($(strip $(WIZARDS_TEMPLATES_DIR)),)
@$(MSG) "Generate DSM Wizards from templates"
@mkdir -p $(WIZARDS_DIR)

This seems to be the new bits that buid the wizards from templates using mustache and outputs them into WIZARDS_DIR.

Then we have this bit that seems to be putting it all together:

spksrc/mk/spksrc.spk.mk

Lines 436 to 440 in ecce16f

ifneq ($(strip $(WIZARDS_DIR)),)
@$(MSG) "Create DSM Wizards"
$(eval SPK_CONTENT += WIZARD_UIFILES)
@mkdir -p $(DSM_WIZARDS_DIR)
@find $${SPKSRC_WIZARDS_DIR} -maxdepth 1 -type f -and \( $(WIZARD_FILE_NAMES) \) -print -exec cp -f {} $(DSM_WIZARDS_DIR) \;

The last line seems to be copying the generated files in WIZARDS_DIR (which is the same as SPKSRC_WIZARDS_DIR) and placing them into DSM_WIZARDS_DIR to join what is already there.

Would it make sense then that to fix this issue that we add a line right after this to look for a file named uninstall_uifile.sh and if present, remove the file uninstall_uifile that was copied in the first set of code above? Perhaps something like:

@if [ -f "$(DSM_WIZARDS_DIR)/uninstall_uifile.sh" ] && [ -f "$(DSM_WIZARDS_DIR)/uninstall_uifile" ]; then \
    rm "$(DSM_WIZARDS_DIR)/uninstall_uifile"; \
fi

@mreid-tt
Copy link
Contributor Author

@hgy59 based on your comment in #5913, I've rebased this PR to incorporate your fix. Should I test by installing the current ownCloud release in DSM 6 and then attempting a package update using the output from this build? I only checked a package update in DSM 7 before.

@hgy59
Copy link
Contributor

hgy59 commented Oct 17, 2023

@hgy59 based on your comment in #5913, I've rebased this PR to incorporate your fix. Should I test by installing the current ownCloud release in DSM 6 and then attempting a package update using the output from this build? I only checked a package update in DSM 7 before.

As I wrote elsewhere it is not so easy to migrate to the new shared folder handling for DSM 6 (and the same might apply when migrating from DSM 6 to 7).
On PR #5213 the only solution I found so far is, to present an upgrade wizard with the shared folder name as default. This can be hidden when the resource worker is already used for the share.
The related code in upgrade_uifile.sh looks like:

#!/bin/sh

# provide an upgrade wizard if the shared folder is not already migrated to resource worker
RESOURCE_OWN="/var/packages/${SYNOPKG_PKGNAME}/conf/resource.own"
if [ -r ${RESOURCE_OWN} ]; then
  if [ $(cat ${RESOURCE_OWN} | jq '."data-share".shares[0].created') == "true" ]; then
    # shared folder already migrated to resource worker
    exit 0;
  fi
else
  # shared folder not supported by resource worker (DSM 5, SRM)
  exit 0;
fi

# Migration of shared folders requires wizard variable referenced by resource file
INST_ETC="/var/packages/${SYNOPKG_PKGNAME}/etc"
INST_VARIABLES="${INST_ETC}/installer-variables"

# Recreate wizard variable from SHARE_NAME or SHARE_PATH
if [ -r "${INST_VARIABLES}" ]; then
   while read -r _line; do
      _key=${_line%%=*}
      _value=${_line#*=}
      declare -g "${_key}=${_value}"
   done < ${INST_VARIABLES}
fi

if [ -z "${SHARE_NAME}" ]; then
  if [ -z "${SHARE_PATH}" ]; then
    exit 0;
  fi
  declare -g SHARE_NAME=$(basename ${SHARE_PATH})
fi

cat <<EOF > $SYNOPKG_TEMP_LOGFILE
[
...

the final cat has to create the json for the wizard with the default value for the shared folder name defined as:

             "defaultValue": "${SHARE_NAME}",

I didn't find any other solution yet.
Such solution could be

  • patch conf/resource in the package so that the share name from installer-variables is used by DSM installer
  • avoid the need of an upgrade wizard by definition of the wizard variable for the share name so that it is used by DSM installer

Another solution would be to force the user to uninstall the package first on DSM 6 (or not to migrate to the new shared folder handling)...

@mreid-tt
Copy link
Contributor Author

As I wrote elsewhere it is not so easy to migrate to the new shared folder handling for DSM 6 (and the same might apply when migrating from DSM 6 to 7).

Ah, I understand now. In this case, I don't believe this will be needed for this package. Since I first modernised this package under #5619, it was migrated to shared folder handling.

Additionally, based on the constraints from the original repo, it was not upgradeable from v8.1.1-7 and required the user to uninstall and re-install per this part of the wizard:

# ownCloud upgrades only possible from 8.2.11, 9.0.9, 9.1.X, or 10.X.Y

As such, I believe we are good to go with this PR. Can you give it a final review?

@hgy59
Copy link
Contributor

hgy59 commented Oct 17, 2023

Ah, I understand now. In this case, I don't believe this will be needed for this package. Since I first modernised this package under #5619, it was migrated to shared folder handling.

👍 Yes, owncloud already had the resource worker for DSM 6 (with USE_DATA_SHARE_WORKER = yes). The upgrade issue is related to migration from manually created shares on DSM 6 to resource worker.

@mreid-tt
Copy link
Contributor Author

@hgy59 nice, are we good to merge?

@mreid-tt
Copy link
Contributor Author

@hgy59, I've implemented a new backup and restore function which was missing before but now complete. I've also experimented with some workarounds for different journeys based on the installation type (new or restore), inspired by the MariaDB package scripts. It's a bit of a hack but works well. This should be the final version for review.

@mreid-tt mreid-tt requested a review from hgy59 October 23, 2023 01:30
@mreid-tt mreid-tt mentioned this pull request Oct 25, 2023
7 tasks
@mreid-tt mreid-tt requested a review from th0ma7 October 25, 2023 19:16
@mreid-tt mreid-tt removed request for hgy59 and th0ma7 November 2, 2023 12:04
@mreid-tt mreid-tt merged commit c33c1a2 into SynoCommunity:master Nov 2, 2023
17 checks passed
@mreid-tt mreid-tt deleted the owncloud-update branch November 2, 2023 12:07
@mreid-tt mreid-tt added status/published Published and activated (may take up to 48h until visible in DSM package manager) and removed status/needs-review labels Nov 2, 2023
@mreid-tt mreid-tt mentioned this pull request Nov 8, 2023
6 tasks
@mreid-tt mreid-tt removed the status/published Published and activated (may take up to 48h until visible in DSM package manager) label Nov 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants