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

Check for more php modules #40889

Merged
merged 4 commits into from Oct 26, 2023
Merged

Check for more php modules #40889

merged 4 commits into from Oct 26, 2023

Conversation

come-nc
Copy link
Contributor

@come-nc come-nc commented Oct 12, 2023

Summary

Follow-up PR for #32550

Checklist

@come-nc come-nc added this to the Nextcloud 28 milestone Oct 12, 2023
@come-nc come-nc self-assigned this Oct 12, 2023
@come-nc come-nc force-pushed the feat/api-cleanup-check branch 4 times, most recently from 1db2ae9 to a3cc3b1 Compare October 19, 2023 09:44
Base automatically changed from feat/api-cleanup-check to master October 19, 2023 13:41
Test all modules listed as required in our documentation

Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nc come-nc force-pushed the feat/check-for-more-php-modules branch from f2348f4 to 0c34684 Compare October 19, 2023 13:58
@come-nc come-nc added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Oct 19, 2023
@come-nc come-nc requested review from nickvergessen, a team, icewind1991, blizzz and Altahrim and removed request for a team October 19, 2023 14:16
@come-nc come-nc marked this pull request as ready for review October 19, 2023 14:16
Copy link
Collaborator

@Altahrim Altahrim left a comment

Choose a reason for hiding this comment

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

Should we also upgrade this list? https://github.com/nextcloud/server/blob/master/composer.json#L26-L33
I was wondering if it's possible to have this list only once, but I don't know how ^^

@nickvergessen
Copy link
Member

I was wondering if it's possible to have this list only once, but I don't know how ^^

composer.json is shipped now I think, so could read it and json_decode() and then access the list?

Problem with this could be that it will prevent composing on servers without the package (e.g. on CI), but well we ultimately rely on it anyway.

Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nc
Copy link
Contributor Author

come-nc commented Oct 24, 2023

I was wondering if it's possible to have this list only once, but I don't know how ^^

composer.json is shipped now I think, so could read it and json_decode() and then access the list?

Problem with this could be that it will prevent composing on servers without the package (e.g. on CI), but well we ultimately rely on it anyway.

I updated the list in composer.json but I am a bit uneasy about parsing composer.json from a setup check, can I leave it like that for the time being?

@come-nc come-nc requested a review from susnux October 24, 2023 09:31
@come-nc
Copy link
Contributor Author

come-nc commented Oct 24, 2023

  • CI failure is related

@nickvergessen
Copy link
Member

Btw I just installed bcmath and gmp, but the message does not disappear:

The PHP modules "gmp" and/or "bcmath" are not enabled. If you use WebAuthn passwordless authentication, these modules are required.

$ php --version
PHP 8.1.24 (cli) (built: Oct  6 2023 09:46:42) (NTS)
Copyright (c) The PHP Group
Zend Engine v4.1.24, Copyright (c) Zend Technologies
    with Zend OPcache v8.1.24, Copyright (c), by Zend Technologies
    with Xdebug v3.2.1, Copyright (c) 2002-2023, by Derick Rethans
$ sudo apt install php8.1-bcmath 
Paketlisten werden gelesen… Fertig
Abhängigkeitsbaum wird aufgebaut… Fertig
Statusinformationen werden eingelesen… Fertig
Die folgenden NEUEN Pakete werden installiert:
  php8.1-bcmath
0 aktualisiert, 1 neu installiert, 0 zu entfernen und 0 nicht aktualisiert.
Es müssen 16,5 kB an Archiven heruntergeladen werden.
Nach dieser Operation werden 69,6 kB Plattenplatz zusätzlich benutzt.
Holen:1 https://ppa.launchpadcontent.net/ondrej/php/ubuntu jammy/main amd64 php8.1-bcmath amd64 8.1.24-1+ubuntu22.04.1+deb.sury.org+1 [16,5 kB]
Es wurden 16,5 kB in 0 s geholt (49,9 kB/s).
Vormals nicht ausgewähltes Paket php8.1-bcmath wird gewählt.
(Lese Datenbank ... 223224 Dateien und Verzeichnisse sind derzeit installiert.)
Vorbereitung zum Entpacken von .../php8.1-bcmath_8.1.24-1+ubuntu22.04.1+deb.sury.org+1_amd64.deb ...
Entpacken von php8.1-bcmath (8.1.24-1+ubuntu22.04.1+deb.sury.org+1) ...
php8.1-bcmath (8.1.24-1+ubuntu22.04.1+deb.sury.org+1) wird eingerichtet ...

Creating config file /etc/php/8.1/mods-available/bcmath.ini with new version
Trigger für libapache2-mod-php8.1 (8.1.24-1+ubuntu22.04.1+deb.sury.org+1) werden verarbeitet ...
Trigger für php8.1-fpm (8.1.24-1+ubuntu22.04.1+deb.sury.org+1) werden verarbeitet ...
NOTICE: Not enabling PHP 8.1 FPM by default.
NOTICE: To enable PHP 8.1 FPM in Apache2 do:
NOTICE: a2enmod proxy_fcgi setenvif
NOTICE: a2enconf php8.1-fpm
NOTICE: You are seeing this message because you have apache2 package installed.
Trigger für php8.1-cli (8.1.24-1+ubuntu22.04.1+deb.sury.org+1) werden verarbeitet ...
$ sudo apt install php8.1-gmp
Paketlisten werden gelesen… Fertig
Abhängigkeitsbaum wird aufgebaut… Fertig
Statusinformationen werden eingelesen… Fertig
php8.1-gmp ist schon die neueste Version (8.1.24-1+ubuntu22.04.1+deb.sury.org+1).
0 aktualisiert, 0 neu installiert, 0 zu entfernen und 0 nicht aktualisiert.

Do they need to be more configured then just installing? If so we should document/outline this?

But mostlikely unrelated to this PR here

@come-nc
Copy link
Contributor Author

come-nc commented Oct 24, 2023

Our documentation about that could be more complete, or give pointers to other documenations: https://docs.nextcloud.com/server/latest/admin_manual/installation/source_installation.html#prerequisites-for-manual-installation

@nickvergessen According to comments on https://www.php.net/manual/en/gmp.installation.php , it either has something to do with reloading apache service, or uncommenting the extension in a php.ini (but be careful there’s one for cli and one for apache from what I recall).

The extensions you talk about are from areWebauthnExtensionsEnabled check, which is not migrated to the new API yet.
But still, it is relevant to this PR that we should link to documentation from php modules setup check when it fails. (and that means we need a relevant documentation page, which is not really the case yet)

@come-nc
Copy link
Contributor Author

come-nc commented Oct 24, 2023

@nickvergessen nextcloud/documentation#11239 something like that would give us a documentation page to link to, what do you think?
The modules part is moved from another page and not changed a lot yet, but if you figure out what was missing in your case we can add it to the page.

Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nc
Copy link
Contributor Author

come-nc commented Oct 26, 2023

Added links to documentation to admin-php-modules which will need to be added to https://github.com/nextcloud/documentation/blob/master/go.php in a follow up.

@come-nc come-nc added the pending documentation This pull request needs an associated documentation update label Oct 26, 2023
@come-nc come-nc merged commit d0ed5ea into master Oct 26, 2023
36 of 40 checks passed
@come-nc come-nc deleted the feat/check-for-more-php-modules branch October 26, 2023 17:13
@MichaIng
Copy link
Member

MichaIng commented Nov 19, 2023

Probably the wrong place to ask, but since I see bz2 popping up as missing module after this commit: Does anyone know whether this is really used, even if installed and enabled? I see that it's internally supported here, but I do not see a logic which would e.g. automatically download bz2 instead of gz archives of Nextcloud or apps, if the module is present. Also all apps I checked are offered as gzip archives only. The Nextcloud server is offered as bzip2 archive as well, though.
EDIT: At least the NC updater downloads the zip archive, also after installing+enabling the bz2 module.

@come-nc
Copy link
Contributor Author

come-nc commented Nov 20, 2023

Probably the wrong place to ask, but since I see bz2 popping up as missing module after this commit: Does anyone know whether this is really used, even if installed and enabled? I see that it's internally supported here, but I do not see a logic which would e.g. automatically download bz2 instead of gz archives of Nextcloud or apps, if the module is present. Also all apps I checked are offered as gzip archives only. The Nextcloud server is offered as bzip2 archive as well, though. EDIT: At least the NC updater downloads the zip archive, also after installing+enabling the bz2 module.

Hum I don’t know, I took the recommended list from the documentation.
It is very possible that application in bz2 is a feature which no application use 🤷

@MichaIng
Copy link
Member

MichaIng commented Dec 8, 2023

I just checked the developer docs:

So the release process docs explicitly say ".tar.gz tarball" and the release workflow example also uses a .tar.gz tarball only. So if .tar.bz2 was a supported alternative, it is at least not documented. Furthermore I do not see much use for bzip2 nowadays, it is slower + lower compression ratio than alternatives like xz, which is the standard/default for Linux (releases), APT (packages + package lists) and many other projects nowadays.

So it makes sense to either remove bzip2 support (and hence the module) from the docs completely, or keep the infrastructure for multiple compression algorithm support (in case it does even work OOTB with bzip2 compressed app tarballs) and document it + by times add xz as another more reasonable alternative (bandwidth/storage wise).

I'll open a new issue about this. Minor importance probably, but I like to keep our project's Nextcloud implementation slim (no useless PHP modules) without users asking/complaining about admin panel warnings.

@J0WI
Copy link
Contributor

J0WI commented Dec 16, 2023

Same question was raised in nextcloud/docker#1351 (comment) and nextcloud/docker#2118. I'm also interested to sort this out, let me know when you created the issue.

@joshtrichards
Copy link
Member

Issue #42342 created for Server w/ summary of issues to hopefully get this addressed once and for all across server,appstore, documentation, and docker. :)

@J0WI J0WI mentioned this pull request Jan 15, 2024
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews enhancement pending documentation This pull request needs an associated documentation update
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants