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

Fix installation error "Unterminated preprocessor conditions" in php 8.3 #641

Conversation

FedericoHeichou
Copy link

@FedericoHeichou FedericoHeichou commented Nov 24, 2023

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

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

//#endif
), but I think is ok to use this patch because it keep the same 3.7.0 logics (everything after line
#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
Copy link

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
Copy link

Can this please be merged?

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

Thank you

@dantheman2865
Copy link

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
Copy link

en-jschuetze commented Nov 30, 2023

I guess it is fixed already on master at

3.7.0...master#diff-f25e44a5ebd6936312bc52ad6c2fd5b830e30300b56457baec2168e59156269cR1272

image

but didn't make it into the release yet.

The commit was at 5ae2ecf

@FedericoHeichou
Copy link
Author

I guess it is fixed already on master at

3.7.0...master#diff-f25e44a5ebd6936312bc52ad6c2fd5b830e30300b56457baec2168e59156269cR1272

image

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
Copy link

jderusse commented Dec 1, 2023

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

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

@FedericoHeichou
Copy link
Author

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
Copy link

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

aran112000 added a commit to aran112000/extra-php-extensions that referenced this pull request Dec 6, 2023
@adamasantares
Copy link

adamasantares commented Dec 8, 2023

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

@FedericoHeichou
Copy link
Author

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

It is not possible, you did something else wrong

@mathroc
Copy link

mathroc commented Dec 8, 2023

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

xarem added a commit to whatwedo/docker-base-images that referenced this pull request Dec 9, 2023
remove this after Imagick/imagick#641 has been
merged and released.
andytson-inviqa added a commit to my127/docker-php that referenced this pull request Dec 13, 2023
a major change for 8.3 docker tags onwards:
* nodejs no longer will be set up with a default version, however nvm
will remain to allow installation
* mcrypt no longer will be enabled by default, as it is long past EOL,
and frameworks must have moved away from it by now

extensions not yet supported:
* imagick - bug fix `Imagick/imagick#641 needed
* mcrypt - needs a release allowing 8.3.0 stable
@bulatovv
Copy link

bulatovv commented Dec 14, 2023

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

@Orkin
Copy link

Orkin commented Dec 19, 2023

Any plan about when this PR will be merged ?

@schmunk42
Copy link

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

@breart
Copy link

breart commented Dec 20, 2023

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
Copy link

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)

@Wirone
Copy link

Wirone commented Feb 2, 2024

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 😅).

PS. The saddest part is that it seems the project is dead because its maintainer is not responding on any platform 😞.

@bilelbekkouche
Copy link

Hello, do we know when the fix will be merged onto the main branch?

@renky
Copy link

renky commented Feb 6, 2024

Hmm... but @Danack is active right here in github - he commited to php-src and also created issues - for example here: https://github.com/Danack/DataType/commits/main/
Indeed - not very acitve but also not completly gone...

So @Danack maybe you can share your thoughts? How will it go on with imagick-extension? If it is dead, then please add maintainer-wanted topic - due to the popularity of this extension I can imagine that somebody will continue...

@DracoBlue
Copy link

DracoBlue commented Feb 7, 2024

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
Copy link

TildBJ commented Feb 7, 2024

I guess it is fixed already on master at

3.7.0...master#diff-f25e44a5ebd6936312bc52ad6c2fd5b830e30300b56457baec2168e59156269cR1272

image

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
image

Therefore its already fixed, just not released yet.

@FedericoHeichou
Copy link
Author

FedericoHeichou commented Feb 9, 2024

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
Copy link

renky commented Feb 9, 2024

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

andrii-kryvoviaz added a commit to andrii-kryvoviaz/symfony-swoole-runtime that referenced this pull request Feb 17, 2024
Remove imagick extension, since it has a bug (see Imagick/imagick#641)
@gdbjohnson
Copy link

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

@orange181
Copy link

orange181 commented Mar 1, 2024

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
Copy link

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

@renky
Copy link

renky commented Mar 1, 2024

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
Copy link

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

@drazenbebic
Copy link

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

@tbreuss
Copy link

tbreuss commented Mar 6, 2024

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
Copy link

Wirone commented Mar 6, 2024

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
Copy link

Orkin commented Mar 6, 2024

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
Copy link

fragkp commented Mar 6, 2024

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
Copy link

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
Copy link

when will be it fixed?

@Erik-Hoffmann
Copy link

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

@Wirone
Copy link

Wirone commented Mar 25, 2024

@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
Copy link
Author

FedericoHeichou commented Mar 25, 2024

@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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Install error "Unterminated preprocessor conditions" in php 8.3