Imagick
Fix installation error "Unterminated preprocessor conditions" in php 8.3
The first if condition in Imagick.stub.php is not closed
https://github.com/Imagick/imagick/blob/28f27044e435a2b203e32675e942eb8de620ee58/Imagick.stub.php#L5-L8
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 https://github.com/Imagick/imagick/blob/52ec37ff633de0e5cca159a6437b8c340afe7831/Imagick.stub.php#L1249), but I think is ok to use this patch because it keep the same 3.7.0 logics (everything after line https://github.com/Imagick/imagick/blob/52ec37ff633de0e5cca159a6437b8c340afe7831/Imagick.stub.php#L1236 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
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
Can this please be merged?
All our pipelines are failing building Docker Images because of this.
Thank you
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
I guess it is fixed already on master at
https://github.com/Imagick/imagick/compare/3.7.0...master#diff-f25e44a5ebd6936312bc52ad6c2fd5b830e30300b56457baec2168e59156269cR1272
but didn't make it into the release yet.
The commit was at https://github.com/Imagick/imagick/commit/5ae2ecf20a1157073bad0170106ad0cf74e01cb6
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
are you sure about your PR? I counted 130 #if and 131 #endif
The first #if MagickLibVersion > 0x628 is closed at line 117
are you sure about your PR? I counted 130
#ifand 131#endifThe first
#if MagickLibVersion > 0x628is 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
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
still doesn't work, I had to roll back to php 8.2
It is not possible, you did something else wrong
I can confirm that I was able to build imagick for php 8.3 arch64 using this branch :ok_hand:
I've got it up and running smoothly for PHP 8.3! Can't wait for the merge :pray:
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?
UPD. Just tried building
masterwith 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 https://github.com/Imagick/imagick/pull/641#issuecomment-1833896254
The missing #endif has already been fixed in 5ae2ecf20a1157073bad0170106ad0cf74e01cb6, 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 https://github.com/Imagick/imagick/pull/641#issuecomment-1836340413
That makes sense 👍
Mystery solved then, the thread is super confusing. This PR shouldn't be merged then, we simply need a release. @Danack would you be able to do it?
That makes sense 👍
Mystery solved then, the thread is super confusing. This PR shouldn't be merged then, we simply need a release. @Danack would you be able to do it?
Idk, this is a non-breaking change for current stable, if master is not tagged I think there is a reason. This PR can be still backported to 3.7.x, maybe the new tag in master would be 3.8.0.
Probably should not be merged in master.
We can just wait for authors, I didn't check at all master branch, I just debugged the code and the first if in 3.7.0 is definitely not closed and it must be closed in the last lines, then I merged this patch in my local build into 3.7.0's code.
the first if in 3.7.0 is definitely not closed and it must be closed in the last lines
in 3.7.0, the first #if is closed at line 117
The missing #endif is the one fixed in 5ae2ecf20a1157073bad0170106ad0cf74e01cb6 at line 1249 Corresponding to the #if opened at line 1236
This PR adds an #endif at the wrong place and has the wrong parent (should be 52ec37ff633de0e5cca159a6437b8c340afe7831 if you don't want merging it in master)
the first if in 3.7.0 is definitely not closed and it must be closed in the last lines
in 3.7.0, the first
#ifis closed at line 117 The missing#endifis the one fixed in 5ae2ecf at line 1249 Corresponding to the#ifopened at line 1236This PR adds an
#endifat the wrong place and has the wrong parent (should be 52ec37f if you don't want merging it in master)
I just checked and you are right, I am sorry.
I know the parent is definitely wrong, I made a mistake while moving the diff from local patch file to GitHub lol.
Anyway, I totally agree that adding the endif there should be the correct fix, but it will drastically change the code because I think in 3.7.0 everything after line 1236 is kept only if that condition is matched. Has anyone tested if the fix doesn't brake anything? If it is already merged I suppose yes, but I personally didn't.
I'll try tomorrow to change the endif and parent commit, this won't be merged in master but at least I will try provide a correct working branch for a hypothetical 3.7.1, because many people linked this PR and if this is wrong could be a problem
Edit: NVM I checked the full commit and the author changed many things (probably adding the correct endif breaks things).
I won't force push anything because it may not work and is not worth, I personally prefer keeping a wrong end if but keep the code as it is.
Would still better close this PR and use the master one, but I will wait for something official before closing it by myself
I tried building imagick from the master branch and haven't found anything glaringly broken so far.
This thread is confusing to say the least; some say to use the master branch (as it's already fixed there) and others are seemingly waiting for this PR to be merged.
One way or another, seems a new release is needed, so please do.
This thread is confusing to say the least; some say to use the master branch (as it's already fixed there) and others are seemingly waiting for this PR to be merged.
One way or another, seems a new release is needed, so please do.
This is a monkey patch and should not be definitely merged in master, maybe could be rebased, merged and tagged in a specific branch if master is not safe to use (as I did on my local repo).
The better solution would be to tag master, but I started thinking that this extension is dead
Firstly I wait as well for a fixed release of this extension compatible with PHP 8.3. Still this PR is not needed and just causes confusion. It should be closed and not being the entry point for anyone waiting the fixed release which it seems would just be a new tag of the master.
With the last release now being over 2 years ago, I don't have much hope of this getting a new release at all.
Does the extension need new maintainers? 2 years is quite a while for such a broadly used extension
