Fix installation error "Unterminated preprocessor conditions" in php 8.3

[FedericoHeichou]

The first if condition in Imagick.stub.php is not closed

imagick/Imagick.stub.php

Lines 5 to 8 in 28f2704

	class Imagick
	{
	#if MagickLibVersion > 0x628
	public function optimizeImageLayers(): bool {}

This causes a installation error "Unterminated preprocessor conditions" in PHP 8.3

Resolves #640

Edit: this branch uses the wrong parent (it starts from master instead of 3.7.0) furthermore the endif I added is technically in wrong position (the correct one is commented here

imagick/Imagick.stub.php

Line 1249 in 52ec37f

//#endif

), but I think is ok to use this patch because it keep the same 3.7.0 logics (everything after line

imagick/Imagick.stub.php

Line 1236 in 52ec37f

#if MagickLibVersion > 0x628

is inside the if that in 3.7.0 is not closed).

If you want to use it I suggest to get the diff file and apply it directly to the 3.7.0 code (you can download it runtime even with pecl and apply the patch).

Anyway this should not be merged in master, I'll keep this open just to keep this problem in evidence

[francoism90]

Could this please be merged? :)

Saved /tmp/pear/temp/imagick/ImagickDraw_arginfo.h
Parse /tmp/pear/temp/imagick/ImagickPixelIterator.stub.php to generate /tmp/pear/temp/imagick/ImagickPixelIterator_arginfo.h
Saved /tmp/pear/temp/imagick/ImagickPixelIterator_arginfo.h
Parse /tmp/pear/temp/imagick/ImagickPixel.stub.php to generate /tmp/pear/temp/imagick/ImagickPixel_arginfo.h
Saved /tmp/pear/temp/imagick/ImagickPixel_arginfo.h
Parse /tmp/pear/temp/imagick/Imagick.stub.php to generate /tmp/pear/temp/imagick/Imagick_arginfo.h
In /tmp/pear/temp/imagick/Imagick.stub.php:
Unterminated preprocessor conditions
make: *** [Makefile:196: /tmp/pear/temp/imagick/Imagick_arginfo.h] Error 1
ERROR: `make INSTALL_ROOT="/tmp/pear/temp/pear-build-defaultuserfiheGA/install-imagick-3.7.0" install' failed

[K2ouMais]

Can this please be merged?

All our pipelines are failing building Docker Images because of this.

Thank you

[dantheman2865]

Until this gets merged, I am using a workaround in my automated builds to explicitly fix the PHP version to 8.2. This is suggested by Composer:

PHP version & extensions

(optimal) create your own build image and install Composer inside it.
Note: Docker 17.05 introduced multi-stage builds, simplifying this enormously:
COPY --from=composer /usr/bin/composer /usr/bin/composer

In my Dockerfile I had:

FROM composer:2.5 as composer_base

I've updated it to copy Composer from their image into a PHP 8.2 image in order to build with that version:

FROM composer:2.5

# TODO: Revert back to image `composer:2.5`
FROM php:8.2-alpine as composer_base
COPY --from=composer /usr/bin/composer /usr/bin/composer

[en-jschuetze]

I guess it is fixed already on master at

3.7.0...master#diff-f25e44a5ebd6936312bc52ad6c2fd5b830e30300b56457baec2168e59156269cR1272

but didn't make it into the release yet.

The commit was at 5ae2ecf

[FedericoHeichou]

I guess it is fixed already on master at

3.7.0...master#diff-f25e44a5ebd6936312bc52ad6c2fd5b830e30300b56457baec2168e59156269cR1272

but didn't make it into the release yet.

The commit was at 5ae2ecf

No, it is a commit for a typo in another non-merged commit. Check the current file in master, that endif is for another "if". The problematic endif is the one (missing) which should close the first if opened in the file. I found out debugging the script which processes those files, it now keep tracks of every opened "if" like a stack

[jderusse]

are you sure about your PR? I counted 130 #if and 131 #endif

The first #if MagickLibVersion > 0x628 is closed at line 117

[FedericoHeichou]

are you sure about your PR? I counted 130 #if and 131 #endif

The first #if MagickLibVersion > 0x628 is closed at line 117

Yes I am, I'm currently using this patch, I debugged it adding var_dump in the function which checks if those comments are correctly closed.

https://github.com/php/php-src/blob/d26068059e83fe40de3430a512471d194119bee0/build/gen_stub.php#L3961

If you want to try it download the diff file, create your own package and try it (not sure if commands are correct, this is just a example, I have already a tgz and I use it)

pecl download imagick
tar xzf imagick-3.7.0.tgz
patch mypatch.diff imagick/Imagick.stub.php
# get md5 of that patched file and sed it in package.xml
tar czf imagick-3.7.0-patch01.tgz imagick package.xml
pecl install ./imagick-3.7.0-patch01.tgz

[aran112000]

Hi @Danack, could you please review and merge this if you're happy? It's a 1 line change that fixes support for the recent stable PHP 8.3 release

[adamasantares]

still doesn't work, I had to roll back to php 8.2

[FedericoHeichou]

still doesn't work, I had to roll back to php 8.2

It is not possible, you did something else wrong

[mathroc]

I can confirm that I was able to build imagick for php 8.3 arch64 using this branch 👌

[bulatovv]

I've got it up and running smoothly for PHP 8.3! Can't wait for the merge 🙏

[Orkin]

Any plan about when this PR will be merged ?

[schmunk42]

How about starting with the approval of the workflow-action :) ?

[breart]

Weird, but I'm getting an error when trying to build imagick with this fix:

9.222 In /tmp/imagick/Imagick.stub.php:
9.222 Encountered #endif without corresponding #if
9.227 make: *** [Makefile:196: /tmp/imagick/Imagick_arginfo.h] Error 1

The code I'm using:

RUN git clone https://github.com/Imagick/imagick.git --depth 1 /tmp/imagick && \
    cd /tmp/imagick && \
    git fetch origin pull/641/head:fix-unterminated-preprocessor-conditions && \
    git switch fix-unterminated-preprocessor-conditions

RUN cd /tmp/imagick && \
    phpize && \
    ./configure && \
    make && \
    make install

RUN docker-php-ext-enable imagick

UPD. Just tried building master with the same approach — worked fine. So I guess the problem is not with #endif?

[jderusse]

UPD. Just tried building master with the same approach — worked fine. So I guess the problem is not with #endif?

I have the same issue.

This is what @en-jschuetze said in #641 (comment)

The missing #endif has already been fixed in 5ae2ecf, but has not yet been released.

This PR re-add a new #endif, this is why the file (in this PR) now has more #if than #endif as pointed out in #641 (comment)

[DracoBlue]

For my current needs I am planning to just use the master branch and still call it 3.7.0 to have some parity in version naming.

We kept the pecl install based on imagick and it seems to not fail anymore (at least on the seldom executions we perform at https://github.com/Endava/docker-php right now.

We had the same problem, worked it around by installing Imagick programmatically instead of using PECL (similar to @YannZeRookie but for "official" PHP Docker distribution).

The issue is super confusing since there is no meaningful error and it seems like the buggy behaviour comes from the process that is actually not needed (updating arginfo). To quote @remicollet from room 11:

This is something which happens sometime (file date issue) and build system trying to regenerate arginfo, which is not the proper process for this extension."

Seems like omitting PECL skips that step (or something other (i)magic(k) is happening there 😅).

This sounds like a good way if pecl build is creating transient errors - try without it.

Keep in mind my alpine build for 3.7.0 with aarch64 worked all the time, just the amd64 build was randomly crashing.

*unsubscribing

[TildBJ]

I guess it is fixed already on master at

3.7.0...master#diff-f25e44a5ebd6936312bc52ad6c2fd5b830e30300b56457baec2168e59156269cR1272

but didn't make it into the release yet.

The commit was at 5ae2ecf

I can confirm that this Commit resolved the issue which was introduced with ba2949cb in Imagick.stup.php Line 1388 to 1402

Therefore its already fixed, just not released yet.

[FedericoHeichou]

Guys, Danack is back. He commented in the linked issue.

He is now checking if everything is ok and will tag a version soon. When this will happen and I can confirm everything is ok too I will close this broken PR.

We did it 🙏

[renky]

This are great news - thanks @FedericoHeichou
nevertheless - maybe it would be worth to think about more maintainers for the project and splitting up responsability, since it is used in so much environments... and finally it broke the pecl installation for multiple months now, forcing people to use a direct installation workaround or stick with php 8.2

[gdbjohnson]

I had some trouble on this today, so it seems like the fix is still not released.

[orange181]

Hello everyone,

I was using Imagick to compress and optimize images but with the lack of tracking I migrated to php GD.
It works very well and I'm now on php 8.3 without any weird tricks.

Have a nice day.

[StrangePeanut]

Gmagick allegedly works as a drop-in replacement, but unfortunately some apps specifically require Imagick 😢

[renky]

Well I also think, if this takes longer and longer here, all this apps will start switching... in most cases it's already today possible to switch between gd and imagick in most plugins - I also switched to gd in meanwhile... and as soon as the used libraries support gmagick, I'll switch again...

[thannaske]

Also affected by the issue. How can this be unresolved for over three months now although the issue itself is already fixed?

[drazenbebic]

We are also affected by this, could we get this merged?

[tbreuss]

Also affected by the issue. How can this be unresolved for over three months now although the issue itself is already fixed?

Well, someone has to tag a version. And it seems that the only one, that can do this is @Danack.

[Wirone]

Guys, please stop bumping this, as Danack is aware of this and provided feedback. He worked on the CI setup, it'll be done when it's done. Poking him multiple times won't get us closer, it can actually work the other way around. Just let him finish this when he has time for it (for those who are unaware, as far as I know he had some health problems, so OSS work is not his priority right now, rightfully).

Just leave 👍 reaction in the related issue or here, and wait patiently (or at least ask how you can help with preparing the release, instead of demanding the solution).

PS. @FedericoHeichou maybe it would be better to close this PR as it won't be merged anyway, since the fix is already in the main branch. As of now, it's mostly the place for miscommunication and misunderstanding of the current state of an issue.

[Orkin]

I think we all understand that in this case OSS work is not a priority but in this case maybe the solution would be to find other maintainers to help him doing this. We are many affected by this problem from months and are unable move to php 8.3 specificaly on arm architecture because of this.

[fragkp]

I think we all understand that in this case OSS work is not a priority but in this case maybe the solution would be to find other maintainers to help him doing this. We are many affected by this problem from months and are unable move to php 8.3 specificaly on arm architecture because of this.

You can always use the master branch (this is what we do). Its compatible with PHP 8.3 and works fine.

[schmunk42]

You can always use the master branch (this is what we do). Its compatible with PHP 8.3 and works fine.

Just for the record, that's how we work around this issue in GH-actions and PHP 8.3 only:
yiisoft/yii2-docker@324f7e8...master

[jamesRUS52]

when will be it fixed?

[Erik-Hoffmann]

We also need this fix asap pls, our php8.3 pipelines are crashing due to this 👀

[Wirone]

@jamesRUS52 @Erik-Hoffmann please read this. You also have workarounds provided here and here, so again: read the freaking thread before commenting 🙄.

@FedericoHeichou (or @Danack, sorry for pinging), please let's close this PR as it's misleading people that it requires merge, when it's not.

[FedericoHeichou]

@jamesRUS52 @Erik-Hoffmann please read this. You also have workarounds provided here and here, so again: read the freaking thread before commenting 🙄.

@FedericoHeichou (or @Danack, sorry for pinging), please let's close this PR as it's misleading people that it requires merge, when it's not.

Yes I agree, because Danack seen the thread and the PR.

Let's comment the Issue #640 guys, there are no reasons to keep commenting here.
I close this

[jogoossens]

when is this going to be fixed in a 3.7.1 release?

[DanRaiss]

For anyone facing this issue in your docker

# Imagick is installed from the archive
RUN curl -L -o /tmp/imagick.tar.gz https://github.com/Imagick/imagick/archive/refs/tags/3.7.0.tar.gz \
    && tar --strip-components=1 -xf /tmp/imagick.tar.gz \
    && phpize \
    && ./configure \
    && make \
    && make install \
    && echo "extension=imagick.so" > /usr/local/etc/php/conf.d/ext-imagick.ini \
    && rm -rf /tmp/*
    # <<< End of Imagick installation

[NiklasBr]

That doesn't work for me in Docker, but the many examples in #640 with references to the specific git commit works.