DOMjudge / domjudge-packaging

DOMjudge packaging for (Linux) distributions and live image
31 stars 37 forks source link

Fixup PHP version to 8.? #157

Closed agcom closed 9 months ago

agcom commented 9 months ago

Fix PHP version in the PHP timezone configuration script, used to access PHP's configuration directory.

agcom commented 9 months ago

I had an observation over where else this hard-coded version is used to access the PHP configuration directory; so, grep -rn '/etc/php/':

docker/domserver/configure.sh:26:php_folder=$(echo "/etc/php/8."?"/")
docker/domserver/scripts/start.d/10-timezone.sh:9:php_folder=$(echo "/etc/php/8."?"/")
docker-contributor/Dockerfile:87:        cp -Rf /php-config/* /etc/php/${VERSION}/cli/conf.d; \
docker-contributor/Dockerfile:88:        cp -Rf /php-config/* /etc/php/${VERSION}/fpm/conf.d; \
docker-contributor/scripts/start.sh:28:      "/etc/php/${VERSION}/fpm/php.ini"
docker-contributor/scripts/start.sh:30:      "/etc/php/${VERSION}/cli/php.ini"
docker-contributor/scripts/start.sh:120:  if [[ -f /etc/php/${VERSION}/fpm/pool.d/www.conf ]]
docker-contributor/scripts/start.sh:122:    sudo rm "/etc/php/${VERSION}/fpm/pool.d/www.conf"
docker-contributor/scripts/start.sh:124:  if [[ ! -f /etc/php/${VERSION}/fpm/pool.d/domjudge.conf ]]
docker-contributor/scripts/start.sh:126:    sudo ln -s "${PROJECT_DIR}/etc/domjudge-fpm.conf" "/etc/php/${VERSION}/fpm/pool.d/domjudge.conf"
docker-contributor/scripts/start.sh:129:  sudo sed -i "s/^pm\.max_children = .*$/pm.max_children = ${FPM_MAX_CHILDREN}/" "/etc/php/${VERSION}/fpm/pool.d/domjudge.conf"
docker-gitlabci/Dockerfile:66: && rm /etc/php/*/fpm/pool.d/www.conf

It seems like in other places, a $VERSION variable is used which originates from iterating over the $PHPSUPPORTED variable; grep -rn 'PHPSUPPORTED=':

docker-contributor/Dockerfile:15:  PHPSUPPORTED="7.2 7.3 7.4 8.0 8.1 8.2" \
docker-gitlabci/Dockerfile:3:ENV PHPSUPPORTED="7.2 7.3 8.0 8.1 8.2"`

I was wondering, why this variable is not used in the docker/ directory stuff? Also, if fixating the PHP version inside the docker/ directory stuff is not vital, shouldn't we avoid hard-coding the PHP version?

For example, by iterating over all /etc/php/*/, or extracting the version from php -v.

nickygerritsen commented 9 months ago

I had an observation over where else this hard-coded version is used to access the PHP configuration directory; so, grep -rn '/etc/php/':

docker/domserver/configure.sh:26:php_folder=$(echo "/etc/php/8."?"/")
docker/domserver/scripts/start.d/10-timezone.sh:9:php_folder=$(echo "/etc/php/8."?"/")
docker-contributor/Dockerfile:87:        cp -Rf /php-config/* /etc/php/${VERSION}/cli/conf.d; \
docker-contributor/Dockerfile:88:        cp -Rf /php-config/* /etc/php/${VERSION}/fpm/conf.d; \
docker-contributor/scripts/start.sh:28:      "/etc/php/${VERSION}/fpm/php.ini"
docker-contributor/scripts/start.sh:30:      "/etc/php/${VERSION}/cli/php.ini"
docker-contributor/scripts/start.sh:120:  if [[ -f /etc/php/${VERSION}/fpm/pool.d/www.conf ]]
docker-contributor/scripts/start.sh:122:    sudo rm "/etc/php/${VERSION}/fpm/pool.d/www.conf"
docker-contributor/scripts/start.sh:124:  if [[ ! -f /etc/php/${VERSION}/fpm/pool.d/domjudge.conf ]]
docker-contributor/scripts/start.sh:126:    sudo ln -s "${PROJECT_DIR}/etc/domjudge-fpm.conf" "/etc/php/${VERSION}/fpm/pool.d/domjudge.conf"
docker-contributor/scripts/start.sh:129:  sudo sed -i "s/^pm\.max_children = .*$/pm.max_children = ${FPM_MAX_CHILDREN}/" "/etc/php/${VERSION}/fpm/pool.d/domjudge.conf"
docker-gitlabci/Dockerfile:66: && rm /etc/php/*/fpm/pool.d/www.conf

It seems like in other places, a $VERSION variable is used which originates from iterating over the $PHPSUPPORTED variable; grep -rn 'PHPSUPPORTED=':

docker-contributor/Dockerfile:15:  PHPSUPPORTED="7.2 7.3 7.4 8.0 8.1 8.2" \
docker-gitlabci/Dockerfile:3:ENV PHPSUPPORTED="7.2 7.3 8.0 8.1 8.2"`

I was wondering, why this variable is not used in the docker/ directory stuff? Also, if fixating the PHP version inside the docker/ directory stuff is not vital, shouldn't we avoid hard-coding the PHP version?

For example, by iterating over all /etc/php/*/, or extracting the version from php -v.

For 'production' docker's we want to use one specific version. For contributor (which is a development container) and gitlabci (which is the CI container) we want to install all versions.

But your idea is good, we should set the version once at the top and use that variable everywhere.

agcom commented 9 months ago

So, the PHP version should be fixated in the docker/domserver/Dockerfile (apt installs), and then modify the scripts to retrieve the PHP version by php -v | grep -oP 'PHP \K\d+\.\d+'; right? If so, should I let this pull request go through, or close it in favor of the better solution?

nickygerritsen commented 9 months ago

So, the PHP version should be fixated in the docker/domserver/Dockerfile (apt installs), and then modify the scripts to retrieve the PHP version by php -v | grep -oP 'PHP \K\d+\.\d+'; right?

Yes but we can only apt install a specific version unless we add an apt repo (which is what we do for contrib, but we don't want here).

If so, should I let this pull request go through, or close it in favor of the better solution?

I'm fine with doing either of those. If you would like to try to do the 'fancy' thing that would be awesome.

agcom commented 9 months ago

In that case, the PHP version is fixated by the distribution's repository of the base image; I just suggest to replace hard-coded versions with dynamic ones (e.g. php -v | grep -oP 'PHP \K\d+\.\d+').

But, this fix is also okay until upgrading the base image and the next major PHP version replacing the current one in the distribution's apt repository; also, the fix php -v | grep -oP 'PHP \K\d+\.\d+' might not get accepted, but this has a high chance.

nickygerritsen commented 9 months ago

Yeah let's just merge. CI seems to be an issue with arm64 which I will look into later.