Migrate from php8.0 to php8.1 #2309
Conversation
|
I'm not understanding why we would stop using Line 7 in e419b62 I understood why PHP still had 'php8.0' as it didn't have access to that variable, but with |
|
Sorry, I'm not sure what you mean. Anyway, also note that @binarykitchen proposed to split this pull request up into a version handling bit (see #2308, derived from this work) and a php upgrade bit. |
|
Right @jvolkenant MiaB must depend on a clearly defined PHP version, not on whatever is installed by random on the server. That's why my low-risk PR doesn't do that, nor is upgrading anything. All my PR does it to keep its version consistent across all the code base without having it hardcoded too much. Secondly, there is no need to upgrade PHP to v8.1 right now. Although, I'd like to give credit to @kiekerjan for some inspiration in my other PR. |
|
TLDR; Nextcloud's strict requirements on maximum PHP version has made our Nextcloud upgrades tricky. I don't have an answer, just stating some possibilities. I've had a few thoughts around our potential PHP8.1 and Nextcloud conundrum. This might be a bit long winded so please bear with me. I'm also likely to repeat a lot of what was raised in the other issues and PRs related to this. Nextcloud v27 minimum non-deprecated version of PHP is 8.1. and presumably in a year it will be 8.2 for whatever version is released. This is Nextcloud keeping in step with PHP release cycles (aggressively so?). As we upgrade each previous Nextcloud version through all the current ones, it might be reasoned that users will need to have PHP8.0 installed to upgrade up to Version24 (only applicable if coming from a MIAB version prior to v60 and the current version is on PHP8.1) Options I understood @kiekerjan 's function was to help resolve if we have multiple versions of PHP needed during the installation process so that Nextcloud could be upgraded no matter what version of PHP or Nextcloud database there is or is needed. Possibly the idea being able to determine what version of PHP was installed so the correct commands and sequence get called. Another similar option, having looked at the Nextcloud repo it seems that the version check for the command line is only for versions below 8.0 for v27 and the current master. Perhaps we could use PHP8.0 to do all the upgrades and then finally upgrade to PHP 8.1 (or PHP8.x in the future). This is in a way similar to the function approach but we would hardcode installing, utilising and removing PHP8.0 just for the steps that are needed. Alternatively we could enforce that users have to upgrade to a specific version of MIAB if coming from previous nextcloud ones. I have more to say on this one. Either at the start of the setup scripts or during Nextcloud's one. Or last find an alternative solution to Nextcloud so we aren't tied to their strict PHP versions. I don't think anyone has time to work on an alternative and the scripted migration process needed. If they had it would have probably been done by now :) PHP Versions different classes of users
Hopefully most are in 1 and no one is in 4 but we still need to (should?) allow for these upgrade paths. Bit more on each option Use PHP8.0 and 8.1 during nextcloud upgrade Force users to update to a certain version Updates to the upgrade doco on the website and probably a pinned post on the forum would probably also need to be done for the late comers as that is likely the first places they will look. When we move to 8.2 we may need to include something in the bootstrap to check the version and update it to the latest supported range of pre8.2. This obviously has a fair few drawbacks, and will confuse some people when it comes to updating. Summary Hopefully be corrected on the above and an easy solution is put forward. Also sorry for the long post and thank you for reading. |
|
Thank you for that elaborate post. I don't have a detailed reply on all your arguments right now, but I wanted to add some insight into this pull request.
I think this is what this pu request proposes. By default, php8.1 will be installed from the ubuntu repositories. This works for all software installed. I like the statement on different classes of users
Should have no issues, on update php8.1 is installed, and php8.0 removed
Similare to the previous class. On an update, php8.1 is installed. Php8.0 is already installed, might be used to accomodate nextcloud upgrades, then is removed
Same as previous class
On upgrade, php8.1 is installed. During upgrade of nextcloud, php8.0 is temporarily installed to accomodate upgrades. |
|
One more option that I had proposed during the discussion on upgrading to Jammy is to use docker and the Nextcloud images on docker hub to upgrade the MIAB Nextcloud database. This eliminates all OS/Nextcloud/PHP version issues because the docker image contains everything. The purpose would be to get you to a Nextcloud version that works with the desired PHP installed on the system. The steps are: o install docker, if not already installed At this point owncloud.db and config.php are upgraded to (in this example) Nextcloud 21. Repeat the steps until the database and config.php are upgraded to the desired Nextcloud version. Finally, copy the Nextcloud installation files onto the production system (download and untar), and you're good to go. The whole upgrade process doesn't even have to happen in situ. Copy the database and config to a local machine, run through the docker sequence there, then copy them back. |
|
Regarding this PR... Why not use PHP_VER for That would avoid the hard coded php version numbers in setup. It might be clearer that way. |
I would prefer to see an explicit version number everywhere in case there are multiple versions of PHP installed. |
|
And just to add another option to the mix: Modifying nextcloud's versioncheck.php so as to remove the restriction on PHP8.1 for older nextcloud versions used for the upgrade. Tested this with a test v57a upgrade to v64. Pros: Cons: I haven't done a PR for this as the branch isn't complete by itself, More wanted to demonstrate this as an idea. Happy for someone else to pick it up if it provides inspiration. |
|
@matidau Thanks for giving fresh ideas and listing the Pros + Cons. Regarding "risk appetite", MiaB has always been and must be well-tested for any version bumps of any dependencies. MiaB is about reliable services without excuses. If one email doesn't arrive, goodbye. Our release frequency is low and everyone is busy, so cannot afford regressions. We need more unit tests, automated tests and the missing gaps must be tested by humans. Just adding my two cents. Low risks, please. |
@JoshData I see that you have a different opinion on how multiple versions of php should be handled. I'm willing to put in some more time to realize this, but I would like to ask you for your input beforehand. Are there conditions you want to see before you would consider merging? What kind of things would you reject? |
|
Hello all - what's happening with this PR? Any problems? We need PHP v8.1 soon ... |
|
Ping? |
|
@kiekerjan I see you pushed and rebased. Ready for review and merge? |
|
Hello all, what's up with this PR? It's significant. We need it before upgrading Nextcloud ... |
|
Nothing has happened. I think this needs:
I can do steps 1 and 2, but I'm hesitant to put in the time because of lack of interest. Note that Nextcloud 29 still supports php8.0, so this pull request is not yet necessary. |
|
Thanks, @kiekerjan
Tried a
I am happy to help with tests if you write a test plan.
Would a money donation make a difference?
Well, PHP 8.0 is deprecated for NC v27, and since for MiaB, security is important, we're reluctant to run with a deprecated version, see: |
No, but it's "just work" ;)
This I can do.
Definitely not ;)
Sure, but deprecated "only" means it will disappear in the future.
My conclusions is that we should aim for being able to upgrade PHP to any version currently available. PHP8.1 is only a stepping stone, to be followed by PHP8.2 and maybe even 8.3 somewhere during the coming year. |
|
Thanks for the recent rebase and your above input @kiekerjan
And when you say, we can still bump to Nextcloud v27 using the deprecated PHP v8.0, I remember an older discussion I had with @yodax last year about that, see the other ticket I've created: #2399
Can you please study the exchange I had on that issue 2399 and confirm, it is wise to go ahead upgrading NC to v27 now with a new PR without PHP v8.1? If you think it's fine, then I'm happy to start working on a new PR to bump NC this weekend. Please let me know. |
|
Checked testplan items 1 and 2 with the latest version. |
tools/owncloud-unlockadmin.sh
Outdated
|
|
||
| ADMIN=$(./mail.py user admins | head -n 1) | ||
| ADMIN=$(sudo ./management/cli.py user admins | head -n 1) |
There was a problem hiding this comment.
The cli.py needs admin rights to read certain file. I included it here because it's also on line 23/24 although there it actually mentions a specific user.
We should probably call this file as sudo tools/owncloud-unlockadmin.sh so it's not necessary to have sudo repeated here.
There was a problem hiding this comment.
Hmmm, in theory, and sudo keywords in bash scripts should be avoided. Can you improve this please?
There was a problem hiding this comment.
I'll remove this sudo call.
|
Checked Update test number 1. This works fine, no issues detected. |
z-push admin and top update with PHP_VER change
| @@ -266,6 +266,9 @@ if [ ! -d /usr/local/lib/owncloud/ ] || [[ ! ${CURRENT_NEXTCLOUD_VER} =~ ^$nextc | |||
| apt-get purge -qq -y php8.0 php8.0-fpm php8.0-apcu php8.0-cli php8.0-sqlite3 php8.0-gd \ | |||
| php8.0-imap php8.0-curl php8.0-dev php8.0-xml php8.0-mbstring php8.0-zip \ | |||
| php8.0-common php8.0-opcache php8.0-readline | |||
|
|
|||
| # Unhold packages | |||
| apt-mark unhold php7.0-apcu php7.1-apcu php7.2-apcu php7.3-apcu php7.4-apcu | |||
There was a problem hiding this comment.
I noticed that held packages might prevent the quiet apt installation commands that mailinabox issues, I think specifically in case that the hold package can be updated. To prevent future issues, unhold these packages to be sure.
|
Checked Update test number 2. This works fine, no issues detected. |
|
Finally performed test 5 Upgrade test. No issues detected. |
|
Excellent, thank you so much @kiekerjan I'll review shortly. Have you asked on our Slack channel for feedback as well? Important to work, to communicate together and to keep the ball rolling :) |
| @@ -179,6 +179,9 @@ def wait_for_service(port, public, env, timeout): | |||
| return False | |||
| time.sleep(min(timeout/4, 1)) | |||
|
|
|||
| def get_php_version(): | |||
| return "8.1" | |||
There was a problem hiding this comment.
Could you leave behind a new comment for the next developer that incrementing the version here alone isn't safe? (Like adding instructions how to increment PHP versions the safe way?)
There was a problem hiding this comment.
I add a comment referencing the other location where this is defind
There was a problem hiding this comment.
Pardon, I can't find it?
| @@ -5,7 +5,7 @@ | |||
| # -o pipefail: don't ignore errors in the non-last command in a pipeline | |||
| set -euo pipefail | |||
|
|
|||
| PHP_VER=8.0 | |||
| PHP_VER=8.1 | |||
There was a problem hiding this comment.
Dito. Like mentioned before, it is safe to increment the version from here alone?
I'd add another inline comment here, that the version number is defined in two places.
There was a problem hiding this comment.
I'll add a comment about versioning.
| echo $VERSION > /usr/local/lib/z-push/version | ||
| fi | ||
|
|
||
| # Create admin and top scripts with PHP_VER |
There was a problem hiding this comment.
Curious here, why did you move this code out of the if-else block?
|
These two updates are to address review comments. |
| @@ -138,23 +141,23 @@ InstallNextcloud() { | |||
| if [ -e "$STORAGE_ROOT/owncloud/owncloud.db" ]; then | |||
| # ownCloud 8.1.1 broke upgrades. It may fail on the first attempt, but | |||
| # that can be OK. | |||
| sudo -u www-data php"$PHP_VER" /usr/local/lib/owncloud/occ upgrade | |||
| sudo -u www-data php"$NC_PHP_VER" /usr/local/lib/owncloud/occ upgrade | |||
There was a problem hiding this comment.
Looking ahead to when we upgrade to later versions after 8.1, would it make more sense for nc_php_ver to be a parameter for this function in the same way the hashes are? We could also then include the installation of earlier versions as part of the function instead of the main body of the code. i.e. where nc_php_ver not equals PHP_VER.
There was a problem hiding this comment.
I did think of that, but didn´t want to make more changes then necessary (I like optimizing changes like this, so difficult for me 😉 )
If we make this a function parameter it does make the code clearer I think
There was a problem hiding this comment.
If you have already thought about it, All good by me then.
| @@ -235,10 +255,31 @@ if [ ! -d /usr/local/lib/owncloud/ ] || [[ ! ${CURRENT_NEXTCLOUD_VER} =~ ^$nextc | |||
| InstallNextcloud 24.0.12 7aa5d61632c1ccf4ca3ff00fb6b295d318c05599 4.1.0 697f6b4a664e928d72414ea2731cb2c9d1dc3077 3.2.2 ce4030ab57f523f33d5396c6a81396d440756f5f 3.0.0 0df781b261f55bbde73d8c92da3f99397000972f | |||
| CURRENT_NEXTCLOUD_VER="24.0.12" | |||
| fi | |||
| if [[ ${CURRENT_NEXTCLOUD_VER} =~ ^24 ]]; then | |||
|
|
|||
| if [[ ${CURRENT_NEXTCLOUD_VER} =~ ^2[456] ]]; then | |||
There was a problem hiding this comment.
Should we look look at a function for the removal of the PHP versions, we will need to call an identical or nearly identical code for 8.1 when we move to a higher version. In my mind this would keep the if [[ ${CURRENT_NEXTCLOUD_VER} sections closer to their current format of: check condition, call function.
Note this also could be a future improvement instead of being included in this PR, just thought I would raise it now.
There was a problem hiding this comment.
I thought of doing a separate version, but then I wondered if all dependencies are the same, so I didn´t put in the time to optimize that, just kept it as is.
Again, having two separate functions to install php dependencies and uninstall them, would make the code cleaner / clearer.
There was a problem hiding this comment.
Yeah, perfect later.
But then one of us, or a new developer, might forget? How about adding a TODO comment about it?
There was a problem hiding this comment.
I don´t think we should put TODOs in the code. But I'm only one person 😉
There was a problem hiding this comment.
I don´t think we should put TODOs in the code. But I'm only one person 😉
Was just about to post this.
I think as soon as 8.1 is merged we should put together a PR for 8.2 and we can include this as an improvement.
There was a problem hiding this comment.
I don´t think we should put TODOs in the code. But I'm only one person 😉
Great discussion. It's open source, developers come and leave. Any inline comments, TODO comments or whatever, are precious leftovers for the next developer to pick up.
I've done the same, for example this merged PR of mine, last year when I added plenty of comments:
https://github.com/mail-in-a-box/mailinabox/pull/2268/files
There was a problem hiding this comment.
I think as soon as 8.1 is merged we should put together a PR for 8.2 and we can include this as an improvement.
Ah, PHP v8.2, tell me, why do you think about it? And are there any documentation or TODO comments on how to bump a PHP version?
There was a problem hiding this comment.
Actually, we might be able to skip a php version:
- Create one PR containing upgrades of versions 27, 28 and 29. All are supporting php 8.1
- Create another PR containing upgrade to php 8.3. Supported by nextcloud 28 and further.
- Create another PR optimizing the code (see earlier discussions in this thread)
Update: In hindsight, I'm going too fast. There's are also other applications that depend on PHP, e.g. Roundcube and zpush. And for instance Roundcube dependencies (see stremlau/html5_notifier#57) are also to be taken into account
I use a little table for supported versions:
| Nextcloud version | supported php versions |
|---|---|
| 20 | 7.2, 7.3, 7.4 |
| 21 | 7.3, 7.4, 8.0 |
| 22 | 7.3, 7.4, 8.0 |
| 23 | 7.3, 7.4, 8.0 |
| 24 | 7.4, 8.0, 8.1 |
| 25 | 7.4, 8.0, 8.1 |
| 26 | 8.0, 8.1, 8.2 |
| 27 | 8.0 (d), 8.1, 8.2 (r) |
| 28 | 8.0 (d), 8.1, 8.2 (r), 8.3 |
| 29 | 8.0 (d), 8.1, 8.2 (r), 8.3 |
| 30 | 8.1 (d), 8.2, 8.3 (r) |
| 31 | 8.1 (d), 8.2, 8.3 (r), 8.4 |
Also take into account dependency on the user_external addon
| user_external version | Nextcloud version supported |
|---|---|
| 2.1.0 | 21-22 |
| 3.0.0 | 22-24 |
| 3.1.0 | 22-25 |
| 3.2.0 | 25-27 |
| 3.3.0 | 25-28 |
| 3.4.0 | 25-29 |
This actually leads me to the next bump in the road: support by user_external. Support for newer Nextcloud versions seems to stall But at least by going to version 29 of Nextcloud we get some air in keeping up with the development tempo.
There was a problem hiding this comment.
Excellent summary. We should put these tables somewhere better, maybe under CONTRIBUTING.md?
Once this PR gets merged, I'll create incremental PRs to slowly upgrade our NC versions step by step.
There was a problem hiding this comment.
In the interests of moving this PR forward, can we agree that any additional steps can be left to after the PR?
|
|
||
| ADMIN=$(./mail.py user admins | head -n 1) | ||
| ADMIN=$(./management/cli.py user admins | head -n 1) |
There was a problem hiding this comment.
This is an interesting change. Why rerun whole management-cli for unlocking only?
There was a problem hiding this comment.
I only made the change so it would work. Running the management-cli was already there. I think what this code does is checking that the requested user is actually an Admin according to the Mail-in-a-Box administration, before unlocking the nextcloud.
|
Awesome, well done 🙇 I'm itching to get this out, but double-checked all over again, asked a few more questions ... |
This pull request proposes the upgrade from php8.0 to php8.1. This would remove the dependency on an external ppa.
The logic works by introducing on a number of places a variable containing the php version. This makes the code independent of the used php version.
The upgrade logic is embedded in the nextcloud installation logic. If a version of nextcloud that does not support php8.1 is to be installed, the code will also install php8.0. Once a new enough version of nextcloud is detected, php8.0 is removed.
Installation has been tested via vagrant. Todo: test upgrade from v60 to this, and test upgrade from v57 to this.