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.

Sign up for GitHub

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

PHP 8.3 with Imagick from master #162

Merged
merged 3 commits into from
May 22, 2024
Merged

PHP 8.3 with Imagick from master #162

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
28d5019
Add PHP 8.3 to test workflow
youwe-petervanderwal May 22, 2024
cecaa8d
Install Imagick PECL module from source
youwe-petervanderwal May 22, 2024
a21faab
Build PHP 8.3 images
youwe-petervanderwal May 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .github/workflows/release.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ jobs:
- { tag: 'v3.1', php: '8.2', distro: bookworm, version-override: "", latest-tag: false }
- { tag: 'v3.2', php: '8.2', distro: bookworm, version-override: "", latest-tag: true }
- { tag: '3.x', php: '8.2', distro: bookworm, version-override: "v3-dev", latest-tag: false }
- { tag: '3.x', php: '8.3', distro: bookworm, version-override: "v3-dev", latest-tag: false }

steps:
- uses: actions/checkout@v3
Expand Down
7 changes: 6 additions & 1 deletion .github/workflows/test.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ jobs:
runs-on: ubuntu-latest
strategy:
matrix:
php: [ '8.2' ]
php: [ '8.2', '8.3' ]
distro: [ bookworm ]
steps:
- uses: actions/checkout@v2
Expand All @@ -26,6 +26,11 @@ jobs:
for imageVariant in ${imageVariants[@]}; do
docker build --tag pimcore-image --target="pimcore_php_$imageVariant" --build-arg PHP_VERSION="${{ matrix.php }}" --build-arg DEBIAN_VERSION="${{ matrix.distro }}" .

if [ "$imageVariant" != "min" ]; then
# Test that Imagick is installed
docker run --rm pimcore-image sh -c 'php -m | grep imagick'
fi

if [ "$imageVariant" == "debug" ]; then
# Make sure xdebug is installed and configured on debug-build
docker run --rm pimcore-image sh -c 'php -m | grep xdebug'
Expand Down
13 changes: 11 additions & 2 deletions Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,15 +139,24 @@ RUN set -eux; \
\
pecl install -f \
apcu \
imagick \
# imagick \
redis \
; \
docker-php-ext-enable \
apcu \
imagick \
# imagick \
redis \
; \
\
# Install Imagick from source as long as no official version compatible with PHP 8.3 is released yet
# See https://github.com/Imagick/imagick/issues/640
# Delete and uncomment imagick in the pecl install above when an official version is released
mkdir -p /usr/src/php/ext/imagick; \
# Locking on specific commit hash to provide consistent results, at the moment of writing this is the HEAD of master
curl -fsSL https://github.com/Imagick/imagick/archive/28f27044e435a2b203e32675e942eb8de620ee58.tar.gz | tar xvz -C "/usr/src/php/ext/imagick" --strip 1; \
docker-php-ext-install imagick; \
# End install Imagick from source
\
Comment on lines +154 to +159
Copy link
Member

Choose a reason for hiding this comment

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

LGTM, I was just thinking about making this part PHP 8.3 only, but since we do not touch stable releases (v3.2) it's fine for now as it is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we would go that direction, for code it is probably the cleanest solution to make the download link dependent on the PHP version, so
PHP 8.3: 28f27044e435a2b203e32675e942eb8de620ee58
PHP 8.2: refs/tags/3.7.0

Alternative would be to have a somewhat larger if statement in your Dockerfile

[ "$PHP_VERSION" == "8.2 ] && \
    pecl install -f imagick && \
    docker-php-ext-enable imagick;

[ "$PHP_VERSION" != "8.2 ] && \
    mkdir -p /usr/src/php/ext/imagick; \
    # Locking on specific commit hash to provide consistent results, at the moment of writing this is the HEAD of master
    curl -fsSL https://github.com/Imagick/imagick/archive/28f27044e435a2b203e32675e942eb8de620ee58.tar.gz | tar xvz -C "/usr/src/php/ext/imagick" --strip 1; \
    docker-php-ext-install imagick;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If it would make releasing a new version easier I'm happy to make that change, In that case please let me know which option you'd prefer

Copy link
Member

Choose a reason for hiding this comment

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

@youwe-petervanderwal no, fine for me for now. We'll build only 3.x images for PHP 8.3 for now, let's do some tests first and then do the v.3.3 release. If we see some issues with the current approach, we can change it anytime back to use the alternative approach (I'd prefer the second option then 😉).

Merging now. Images will be ready soon then.

build-cleanup.sh; \
\
ldconfig /usr/local/lib; \
Expand Down
Loading