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

cops: upgrade to 1.4.2 #5817

Merged
merged 27 commits into from
Aug 28, 2023

Conversation

smaarn
Copy link
Contributor

@smaarn smaarn commented Jul 21, 2023

Description

Original project has gone quiet for some time now (few years).

A fork has been created and maintained since which includes an OOM fix when trying to download "big files" and adds support for latest supported PHP versions.

This MR is therefore aiming for

  1. Switching to the fork
  2. Upgrading PHP requirement (7.4) to be usable on DSM 7
  3. Add support for .htaccess migration

Additionally all its wizards are switched to using mustache templates.

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

Type of change

  • Bug fix
  • Package update

@smaarn smaarn requested a review from Diaoul July 22, 2023 11:43
@smaarn smaarn marked this pull request as ready for review July 22, 2023 11:43
@smaarn smaarn marked this pull request as draft July 22, 2023 16:37
@smaarn smaarn mentioned this pull request Jul 22, 2023
3 tasks
@smaarn smaarn force-pushed the feat/cops/upgrade-php-and-fork branch 2 times, most recently from ec35045 to 72fde13 Compare July 22, 2023 18:02
@hgy59 hgy59 mentioned this pull request Aug 4, 2023
@smaarn smaarn force-pushed the feat/cops/upgrade-php-and-fork branch from cfa563a to 4d158ef Compare August 6, 2023 11:06
@hgy59
Copy link
Contributor

hgy59 commented Aug 6, 2023

@smaarn unfortunately the github lint action cannot not find json syntax errors here, as the wizard files are generated upon build...

It would be great to have a syntax check of the generated wizard files, but it is not worth to include npm install jsonlint in the Dockerfile...

EDIT:
we could use jq to validate the generated wizard json files see jqlang/jq#1539

spk/cops/src/service-setup.sh Outdated Show resolved Hide resolved
@hgy59 hgy59 mentioned this pull request Aug 6, 2023
1 task
@hgy59
Copy link
Contributor

hgy59 commented Aug 6, 2023

@smaarn I have now a working version of COPS on DSM 7.2.

The last failure was, that I did not install Apache 2.4.
Without the package, there was an installation error logged in /var/log/synopkgmgr.log with the error "failed to acquire postinst worker" for cops.
Can we add Apache2.4 to "SPK_DEPENDS" ?

@hgy59
Copy link
Contributor

hgy59 commented Aug 6, 2023

@smaarn I have now a working version of COPS on DSM 7.2.

The online viewer is not yet working.
When I click the 👁️ eye icon available for ebup books, I get redirected to a page showing status 500. (the url looks like /cops/epubreader.php?data=214&db=)

The same ebook works with cops 1.1.3-6 on DSM 5.2.

The only error log found is in /var/log/nginx/error.log (ip addresses replaced by {client-ip} and {server-ip})

2023/08/06 16:01:44 [error] 6170#6170: *11699 open() "/var/services/web/favicon.ico" failed (2: No such file or directory), client: {client-ip}, server: , request: "GET /favicon.ico HTTP/1.1", host: "{server-ip}", referrer: "http://{server-ip}/cops/epubreader.php?data=214&db="

@smaarn
Copy link
Contributor Author

smaarn commented Aug 6, 2023

Will be checking for adding a json check. I am wondering if a simple jq processing wouldn't be doing the trick.

spk/cops/src/conf/resource Outdated Show resolved Hide resolved
@hgy59
Copy link
Contributor

hgy59 commented Aug 6, 2023

@smaarn I have now a working version of COPS on DSM 7.2.

The online viewer is not yet working. When I click the 👁️ eye icon available for ebup books, I get redirected to a page showing status 500. (the url looks like /cops/epubreader.php?data=214&db=)

The same ebook works with cops 1.1.3-6 on DSM 5.2.

The only error log found is in /var/log/nginx/error.log (ip addresses replaced by {client-ip} and {server-ip})

2023/08/06 16:01:44 [error] 6170#6170: *11699 open() "/var/services/web/favicon.ico" failed (2: No such file or directory), client: {client-ip}, server: , request: "GET /favicon.ico HTTP/1.1", host: "{server-ip}", referrer: "http://{server-ip}/cops/epubreader.php?data=214&db="

this can be fixed by adding the "zlib" extension for php

got this error with cops 1.1.3 on DSM 6 without zlib extension
clsTbsZip ERROR with the zip archive: Unable to uncompress file "META-INF/container.xml" because extension Zlib is not installed.

@hgy59
Copy link
Contributor

hgy59 commented Aug 6, 2023

@smaarn can we please downgrade the required php to v7.4 (instead of 8.2) to get compatibility with DSM 7.1 and DSM 6?

@smaarn
Copy link
Contributor Author

smaarn commented Aug 6, 2023

@smaarn can we please downgrade the required php to v7.4 (instead of 8.2) to get compatibility with DSM 7.1 and DSM 6?

@hgy59 unfortunately we don't have much choice here. The forked version actually requires PHP 8.2. Another option would be to do this in two steps:

  1. An intermediary version which would be fixing the zlib issue, the installation wizard and OOM (using a patch file)
  2. A final version doing the migration to the fork

WDYT ?

@hgy59
Copy link
Contributor

hgy59 commented Aug 6, 2023

@smaarn can we please downgrade the required php to v7.4 (instead of 8.2) to get compatibility with DSM 7.1 and DSM 6?

@hgy59 unfortunately we don't have much choice here. The forked version actually requires PHP 8.2. Another option would be to do this in two steps:

1. An intermediary version which would be fixing the zlib issue, the installation wizard and OOM (using a patch file)

2. A final version doing the migration to the fork

WDYT ?

Regarding the Changelog, php8 is required for cops >= 1.4 (but not for 1.3.6).

So since the last published version of cops is broken on DSM 6, it would be good to have a fixed version (it is not only the missing zlib extension entry).

@smaarn smaarn force-pushed the feat/cops/upgrade-php-and-fork branch from 43bdd00 to c6c674c Compare August 6, 2023 19:13
@smaarn
Copy link
Contributor Author

smaarn commented Aug 6, 2023

Regarding the Changelog, php8 is required for cops >= 1.4 (but not for 1.3.6).

Alas, it seems that the 1.2.0 is already about migrating to PHP 8.0.

So since the last published version of cops is broken on DSM 6, it would be good to have a fixed version (it is not only the missing zlib extension entry).

That's a fair point. Will be making an intermediary fixed version.

@hgy59
Copy link
Contributor

hgy59 commented Aug 7, 2023

Regarding the Changelog, php8 is required for cops >= 1.4 (but not for 1.3.6).

Alas, it seems that the 1.2.0 is already about migrating to PHP 8.0.

Indeed there is no note in the changelog, that PHP 8 is required, there are only entries about PHP 8 compatibility.
cops 1.3.6 runs successfully with PHP 7.4 (changed php backend from 11 to 8 in the resource file and did a fresh install after removing PHP8.2 package).

EDIT
there is a checkconfig page, that can be showed manually:
cops_1 3 6_with_php_7 4

@smaarn smaarn force-pushed the feat/cops/upgrade-php-and-fork branch 3 times, most recently from 6bcdceb to 95afd18 Compare August 7, 2023 15:46
@smaarn smaarn force-pushed the feat/cops/upgrade-php-and-fork branch from c01ca4c to ebf9b29 Compare August 27, 2023 16:38
@smaarn smaarn marked this pull request as draft August 27, 2023 16:38
@smaarn smaarn changed the title cops: upgrade to 1.3.6 cops: upgrade to 1.4.2 Aug 27, 2023
@smaarn
Copy link
Contributor Author

smaarn commented Aug 27, 2023

Bumped to 1.4.2 which is supposed to officially support PHP 7.4

Generated packages here: https://github.com/smaarn/spksrc/releases/tag/cops-1.4.2-rc-1

Will be testing out the newly generated binaries right away.

@smaarn
Copy link
Contributor Author

smaarn commented Aug 27, 2023

Fixed installer PHP instructions for DSM 7

New binaries: https://github.com/smaarn/spksrc/releases/tag/cops-1.4.2-rc-2

@smaarn smaarn marked this pull request as ready for review August 27, 2023 17:26
@smaarn
Copy link
Contributor Author

smaarn commented Aug 27, 2023

PTAL @hgy59 I upgraded to 1.4.2 and fixed a few mustache issues (basically DSM 6 and DSM 7 weren't appropriately separated)

@smaarn smaarn requested a review from hgy59 August 27, 2023 17:27
@hgy59
Copy link
Contributor

hgy59 commented Aug 27, 2023

@smaarn just a small issue with DSM 6:

the installation logs show:


2023/08/27 20:00:55     ===> Step postinst. STATUS=INSTALL USER=http GROUP=http SHARE_PATH=
2023/08/27 20:00:55     Begin save_wizard_variables
2023/08/27 20:00:55     End save_wizard_variables
chown: missing operand after '/var/packages/cops/etc/installer-variables'
Try 'chown --help' for more information.
2023/08/27 20:00:56     Adding '' to 'http'
2023/08/27 20:00:57     Group Name: [http]
2023/08/27 20:00:57     Group Type: [AUTH_LOCAL]
2023/08/27 20:00:57     Group ID:   [1023]
2023/08/27 20:00:57     Group Members:

So for DSM < 7 we need SERVICE_USER definition, otherwise sc-cops is not created, and not available for shared folder permissions.

Please add this to the Makefile (just above the SERVICE_SETUP = src/service-setup.sh

SERVICE_USER = auto

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.

LGTM now

@smaarn smaarn merged commit 9e8ab22 into SynoCommunity:master Aug 28, 2023
17 checks passed
@smaarn smaarn deleted the feat/cops/upgrade-php-and-fork branch August 28, 2023 05:48
@hgy59 hgy59 added status/published Published and activated (may take up to 48h until visible in DSM package manager) and removed status/to-publish labels Sep 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

2 participants