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

Clarification: Is bz2 still really recommended PHP module? #42342

Closed
joshtrichards opened this issue Dec 17, 2023 · 11 comments · Fixed by #42821
Closed

Clarification: Is bz2 still really recommended PHP module? #42342

joshtrichards opened this issue Dec 17, 2023 · 11 comments · Fixed by #42821
Labels
1. to develop Accepted and waiting to be taken care of 28-feedback bug pending documentation This pull request needs an associated documentation update

Comments

@joshtrichards
Copy link
Member

joshtrichards commented Dec 17, 2023

Is bz2 still really a recommended PHP module or are the docs simply out of date?

#40889 added a check for it and various other modules per the current documentation.

The community Docker image does not have bz2. It's an easy fix if actually recommended, but it seems it may not even be used any longer: #40889 (comment)

There is an open issue in the Docker repo (nextcloud/docker#2118) with over 40 upvotes to fix the warning, but it's unclear at present whether the correct solution is to:

  1. Add it to the Docker image
  2. Remove the check from Server + Update the Documentation

For the record, no one really seemed to care it hasn't been in the image until this check got added to Server. :-)

It came up once a couple years ago in the Docker image for an unrelated situation, and word from the appstore then was that it's not needed any longer at least for apps. So does Server need it?

Ref:

@joshtrichards joshtrichards added bug 0. Needs triage Pending check for reproducibility or if it fits our roadmap pending documentation This pull request needs an associated documentation update labels Dec 17, 2023
@jjasoncool
Copy link

jjasoncool commented Dec 18, 2023

In fact, we need to understand the role of the bz2 module in nextcloud/server, and whether there are alternatives to this module.

I've searched some reference materials, and currently, bz2 seems to excel in compression rates but is not very efficient in terms of performance.

Additionally, it has been removed from the Linux kernel. Currently, besides its use during app installation for decompression, are there any other parts of the system that utilize this functionality?

If so, it should be retained; otherwise, it might simplify the nextcloud installation package or allow those who need it to install it separately.

The above is my opinion.

@joshtrichards joshtrichards added 1. to develop Accepted and waiting to be taken care of and removed 0. Needs triage Pending check for reproducibility or if it fits our roadmap labels Dec 18, 2023
@MichaIng
Copy link
Member

MichaIng commented Dec 20, 2023

See my comment here: #40889 (comment)

  • The Nextcloud server archive (for updates) is provided as bz2 tarball as well, but the updater always pulls the zip instead (why not gz?).
  • For apps (this is what the docs state as reason for the bz2 module), there is not a single reference of bz2 in the developer docs, and I could not find any app provided in another format than gz. Probably an admin with access to the app store could search through the files.
  • Also I could not find any logic in the Nextcloud core code which would do some automatic archive type selection for apps, based on enabled PHP modules. This would have been an explanation for having bz2 as recommended module, but not as required one, and without any user/admin ever reporting that something did not work without this module.

And I agree, if such flexibility regarding the apps archive compression algorithm is wanted, then bzip2 is probably not worth to start with. xz would be a proper alternative, with much better compression-ratio (than gz and bzip2) and very good decompression performance as well. But there is no native PHP module supporting it 🤔.

@casperghst42
Copy link

Upgraded to 28.0.1 and I am still getting:

This instance is missing some recommended PHP modules. For improved performance and better compatibility it is highly recommended to install them: bz2. For more details see the [documentation ↗](https://docs.nextcloud.com/server/28/go.php?to=admin-php-modules).

@MichaIng
Copy link
Member

MichaIng commented Dec 25, 2023

Sure, no one has even decided whether this warning shall be removed or not.

@jjasoncool

Currently, besides its use during app installation for decompression, are there any other parts of the system that utilize this functionality?

Indeed extracting apps from app store is the only place where this API is called, from what I could find: https://github.com/nextcloud/server/blob/master/lib/private/Installer.php#L312

Unless I have overseen something (search did), and no apps on the store are compressed with bzip2, this functionality is unused.

Btw, Merry Christmas to everyone celebrating it, else enjoy the season 🙂.

@jjasoncool
Copy link

Additional supplementary program locations and documentation:
TAR path
https://github.com/nextcloud/server/blob/master/lib/private/Archive/TAR.php#L70

Archive_Tar
https://github.com/nextcloud/server/blob/master/lib/private/Archive/TAR.php#L64
According to file description, it seems that .xz files are also supported.
https://pear.php.net/package/Archive_Tar/

Therefore, I'm contemplating whether maintaining various compression methods, even if not currently in use, serves as a reserved approach. Since in Linux systems, the prevalent file formats are mostly gz or xz, if a unified adoption of zip is considered, this feature might become obsolete. I'm not aware of the original designer's intentions; if anyone knows, perhaps it's worth seeking clarification.

@MichaIng
Copy link
Member

Strangely there seems to be no native PHP xz module, but only 3rd party ones outside of distro repos.

@witzker
Copy link

witzker commented Jan 7, 2024

Hi,
As I'm new with this server stuff, my question I have:
_This instance is missing some recommended PHP modules. For improved performance and better compatibility it is highly recommended to install them: bz2. For more details see the documentation ↗.

Is this needed or not?
If Yes, How can I install the missing module on Debian?
Many THX for help._

@MichaIng
Copy link
Member

MichaIng commented Jan 7, 2024

Is this needed or not?

This is what we are trying to clarify with this issue 😄. No final answer yet, but I am 100% sure that it is at least not really needed for anything and 99% sure that it would not even be used by Nextcloud if installed.

However, if you want to install it on Debian:

apt install php-bz2

and restart PHP-FPM, or Apache if mod_php is used, for the change to take effect.

@tzerber
Copy link

tzerber commented Jan 8, 2024

Hi,
I've paid close attention to my instances and I was "hovering" around the related issues since i discovered that warning (and a workaround for it). From what I can see the module is not needed. MAYBE there's a reason for it to be added to the core check, but all the cases i saw that "need" that PHP module was just to get rid of the warning not to freak people out. There was quite a few people freaking out, but the problem was actually the *.mjs file extension and not php-bz2.
I'd say remove the check.

@MichaIng
Copy link
Member

MichaIng commented Jan 8, 2024

Ah right, now that you mention it, it was required to add mjs to be excluded from webserver rewrites, in case, like other assets. mjs support was actually added in NC27 already: https://docs.nextcloud.com/server/latest/developer_manual/app_publishing_maintenance/app_upgrade_guide/upgrade_to_27.html#id1
But with NC28, some core components started to make use of them, being a breaking change for all non-Apache2 or non-htaccess webserver setups.

I have no time to do a PR now, but to not forget about it, and in case someone else finds time: nextcloud/documentation#11429

@joshtrichards
Copy link
Member Author

For apps (this is what the docs state as reason for the bz2 module), there is not a single reference of bz2 in the developer docs, and I could not find any app provided in another format than gz. Probably an admin with access to the app store could search through the files.

I checked through apps available for >=NC26[1]. None are in bz2 format.

However one app does require bz2: https://github.com/matiasdelellis/facerecognition

It's just a dependency like some other apps have other random dependencies. We don't need to be directly concerned with every app's dependencies here (for the setup checks + admin docs I mean).

[1] https://nextcloudappstore.readthedocs.io/en/latest/restapi.html#api-all-releases

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1. to develop Accepted and waiting to be taken care of 28-feedback bug pending documentation This pull request needs an associated documentation update
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants