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

memcached: upgrade 1.6.9 + DSM7 support #4522

Merged
merged 31 commits into from
Jun 4, 2022

Conversation

smaarn
Copy link
Contributor

@smaarn smaarn commented Mar 26, 2021

Motivation: Being able to use memcache and (while we're at it) use the latest version
Linked issues: None that I know of

Checklist

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

@smaarn smaarn force-pushed the feat/memcache/upgrade-1-6-9 branch from 5fa7481 to 4e52351 Compare March 26, 2021 18:45
@smaarn smaarn marked this pull request as ready for review March 26, 2021 22:46
@smaarn
Copy link
Contributor Author

smaarn commented Mar 28, 2021

@ymartin59 ymartin59 changed the title feat(memcache): upgrade 1.6.9 memcache: upgrade 1.6.9 May 14, 2021
@ymartin59 ymartin59 changed the title memcache: upgrade 1.6.9 memcached: upgrade 1.6.9 May 14, 2021
@smaarn smaarn force-pushed the feat/memcache/upgrade-1-6-9 branch from 4e52351 to 14434a2 Compare October 16, 2021 21:35
@smaarn smaarn force-pushed the feat/memcache/upgrade-1-6-9 branch 2 times, most recently from a72bf77 to 98656e1 Compare November 15, 2021 21:41
@ymartin59
Copy link
Contributor

@hgy59 Hello. May you please help with merge and publish this update?

@hgy59 hgy59 force-pushed the feat/memcache/upgrade-1-6-9 branch from 98656e1 to 3effba7 Compare January 21, 2022 21:07
@smaarn smaarn force-pushed the feat/memcache/upgrade-1-6-9 branch from 3effba7 to b509728 Compare April 3, 2022 17:26
@hgy59
Copy link
Contributor

hgy59 commented Apr 3, 2022

@smaarn can you please remove cross/busybox dependency.
This was the old DSM 5 way in former installer.sh scripts to create system users (and to delete those when the generic service installer was introduced).

I don't know whether this package will be running on DSM 7 (as the root user is not available anymore to install and update packages or start/stop services).

  • the current service_postinst function will fail on DSM 7.
  • at least for DSM 7 we need to define a resource worker for the installation of a web service (you find one example in spk/adminer/src/conf/resource)

EDIT:
OK, my bad, just saw that you are working in the right direction on the cops package 😉

@smaarn smaarn marked this pull request as draft April 10, 2022 19:43
@smaarn
Copy link
Contributor Author

smaarn commented Apr 10, 2022

Flagging this PR as draft until DSM7 related logics are implemented

@smaarn smaarn marked this pull request as ready for review May 1, 2022 17:41
@hgy59
Copy link
Contributor

hgy59 commented May 1, 2022

for cross/memcached/Makefile I am pretty sure that

CONFIGURE_ARGS = ac_cv_c_endian=little

must be different for PPC_ARCHS as those are big endian cpu architecture.

@smaarn smaarn marked this pull request as draft May 3, 2022 05:23
Copy link
Contributor

@hgy59 hgy59 left a comment

Choose a reason for hiding this comment

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

The following commands in service-setup.sh are not allowed on DSM 7 (and variable CONFIG_FILE must be assigned before)

      cp -pv "${CONFIG_DIR}/Memcache.sample.php" "${CONFIG_FILE}"
      chgrp http "${CONFIG_FILE}"
      chmod g+w "${CONFIG_FILE}"

and those are the errors in the installation log:

2022/05/03 20:18:58     Begin service_postinst
2022/05/03 20:18:58     cp: failed to access '/var/services/web/phpMemcachedAdmin/Config/Memcache.php': Permission denied
2022/05/03 20:18:58     chgrp: cannot access '': No such file or directory
2022/05/03 20:18:58     chmod: cannot access '': No such file or directory
2022/05/03 20:18:58     End service_postinst

@smaarn smaarn force-pushed the feat/memcache/upgrade-1-6-9 branch from 2d2be47 to 101ab90 Compare May 3, 2022 19:04
@smaarn
Copy link
Contributor Author

smaarn commented May 3, 2022

The following commands in service-setup.sh are not allowed on DSM 7 (and variable CONFIG_FILE must be assigned before)

      cp -pv "${CONFIG_DIR}/Memcache.sample.php" "${CONFIG_FILE}"
      chgrp http "${CONFIG_FILE}"
      chmod g+w "${CONFIG_FILE}"

and those are the errors in the installation log:

2022/05/03 20:18:58     Begin service_postinst
2022/05/03 20:18:58     cp: failed to access '/var/services/web/phpMemcachedAdmin/Config/Memcache.php': Permission denied
2022/05/03 20:18:58     chgrp: cannot access '': No such file or directory
2022/05/03 20:18:58     chmod: cannot access '': No such file or directory
2022/05/03 20:18:58     End service_postinst

And I was wondering why upgrades weren't working... Thanks for pointing it out.

@smaarn smaarn marked this pull request as ready for review May 7, 2022 14:00
@smaarn smaarn requested a review from hgy59 May 7, 2022 14:00
@smaarn smaarn changed the title memcached: upgrade 1.6.9 memcached: upgrade 1.6.9 + DSM7 support May 7, 2022
@smaarn smaarn force-pushed the feat/memcache/upgrade-1-6-9 branch from 42c6496 to 3a62fea Compare May 21, 2022 08:38
@smaarn
Copy link
Contributor Author

smaarn commented May 21, 2022

Rebased at head.

All reported issues are now fixed AFAICT.

- update memcached to v1.6.15
- aarch64 patch is not required (memcached/memcached#743 is not an issue)
- update phpMemcachedAdmin to include fix for 1.6.x
- fix service user
@hgy59
Copy link
Contributor

hgy59 commented May 21, 2022

@smaarn my review ended up in da8abee

The remaining issue is that the memcached icon in DSM UI links to the service port instead of phpMemcachedAdmin.

EDIT:
DSM UI icon links correctly to phpMemcachedAdmin with DSM 7, but not with DSM 6.

hgy59 added 2 commits May 21, 2022 15:52
- add Makefile variable DSM_UI_CONFIG for package specific app/config files
- use custom app/config file to link with phpMemcachdAdmin
- update icon
- update config backup/restore and limit to DSM < 7
- set owner of config file on DSM < 7
@@ -7,7 +7,7 @@
# conf/SPK_NAME.sc if SERVICE_PORT and DSM<7
# conf/resource if SERVICE_CERT or DSM7
# app/SPK_NAME.sc if SERVICE_PORT and DSM7
# app/config if DSM_UI_DIR
# app/config if DSM_UI_DIR (may be overwritten by DSM_UI_CONFIG)
Copy link
Contributor

Choose a reason for hiding this comment

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

@ymartin59, @publicarray some time ago we added the generation of app/config files.
For this package we do not want the generated app/config for the service but one for the included phpMemcachedAdmin web interface.

The app/config file is used with DSM < 7 only, as for DSM 7 the web UI is defined in the resources file and app/config seems to be ignored.

So a lot of packages still have a src/app/config file in the spk folder, and we will have to validate, whether those match the generated one. Matching config files could be removed and for different files we have to apply DSM_UI_CONFIG too.

@hgy59
Copy link
Contributor

hgy59 commented May 21, 2022

Now my testing was successful with aarch64-6.1 and x64-7.0.
But I used the default configuration only, and could not verify whether package updates keep the configuration.

hgy59 added 2 commits June 2, 2022 20:54
- update license
- remove autoconf as configure script is included
- avoid default DSM UI icon on DSM 7
@hgy59
Copy link
Contributor

hgy59 commented Jun 2, 2022

@smaarn Did you have a change to test this package? (IMO it is good to merge.)

@smaarn
Copy link
Contributor Author

smaarn commented Jun 3, 2022

@smaarn Did you have a change to test this package? (IMO it is good to merge.)

Will be having a look today. Seems all good to me TBH.

hgy59 added 2 commits June 3, 2022 13:39
- add cross/libmemcached
- enable build of memaslap
- fix build for DSM 7 (issue with newer compilers)
- add patches for correct program name and faster build
- use code optimization
- fix build for OLD_PPC_ARCHS
@smaarn
Copy link
Contributor Author

smaarn commented Jun 3, 2022

Seems that the upgrade process looses the phpMemcached configuration. Since it's not the memcached configuration in itself I'm guessing we can assume it's fine and good to merge.

I will try having a look at the php configuration to have it remain persistent next week.

@hgy59
Copy link
Contributor

hgy59 commented Jun 4, 2022

I will try having a look at the php configuration to have it remain persistent next week.

@smaarn did you find this issue with DSM 6 or DSM 7 (or both)?.

@smaarn
Copy link
Contributor Author

smaarn commented Jun 4, 2022

I will try having a look at the php configuration to have it remain persistent next week.

@smaarn did you find this issue with DSM 6 or DSM 7 (or both)?.

DSM 7 only. TBH it may be a very simple thing to do since the logic is there for DSM 6

hgy59 added 2 commits June 4, 2022 15:06
- remove SYNOPKG_DSM_VERSION_MAJOR check in installers
- use GROUP variable for group in set_unix_permissions for DSM 6
- update phpmemcachedadmin to store config in var folder and link as Config into web folder
- fix folder permissions for phpmemcachedadmin
- create Memcache.php on demand
@hgy59
Copy link
Contributor

hgy59 commented Jun 4, 2022

@smaarn it was not so easy to create the config on demand for DSM7 and add save/restore on package updates.
It actually required a fix in the generic installer for DSM 6.

@hgy59 hgy59 force-pushed the feat/memcache/upgrade-1-6-9 branch from 85cbead to ac1ae8c Compare June 4, 2022 14:48
@hgy59 hgy59 merged commit 5ce6622 into SynoCommunity:master Jun 4, 2022
@hgy59 hgy59 added framework status/published Published and activated (may take up to 48h until visible in DSM package manager) labels Jun 4, 2022
@smaarn smaarn deleted the feat/memcache/upgrade-1-6-9 branch June 7, 2022 06:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
framework status/published Published and activated (may take up to 48h until visible in DSM package manager)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants