feat: add support for scalingo-26#597
Conversation
e4dc31d to
c73ce92
Compare
84c47c8 to
f24773f
Compare
Frzk
left a comment
There was a problem hiding this comment.
I have a few concerns regarding tidy and gmp.
And another detail related to amqp (we may have to package this extension).
Else LGTM, nice work <3
|
|
||
| if [ "$STACK" = "scalingo-26" ] && [ "${zend_api_version}" -ge "${PHP_MODULE_API_VERSIONS['8.5']}" ]; then | ||
| echo "Apply the patch to fix a call to IS_INTERNED" | ||
| patch -p1 < /buildpack/support/ext/mongodb-is-interned.patch |
There was a problem hiding this comment.
praise: I love how you handled this <3
| mkdir -p /app/vendor/gmp | ||
| pushd "gmp-${gmp_version}" > /dev/null | ||
| ./configure --prefix=/app/vendor/gmp | ||
| # We need the `CC` flag since gcc v15 (used in scalingo-26). |
There was a problem hiding this comment.
question: shall we have a conditional here for stacks < scalingo-26? (or does it work?)
There was a problem hiding this comment.
Even though the default changed in gcc, the flag -std=gnu17 is supported by all gcc versions since a long time.
I double checked by executing this script in the scalingo-22 container and I confirm it works.
There was a problem hiding this comment.
question: this file is not required anymore for scalingo-26, but is it for older stacks?
There was a problem hiding this comment.
question: this file is not required anymore for scalingo-26, but is it for older stacks?
There was a problem hiding this comment.
I renamed this file to get_libtidy but it eventually still exists.
| --with-pdo-sqlite | ||
| --with-pgsql | ||
| --with-readline | ||
| --with-tidy |
There was a problem hiding this comment.
question: is this option valid for PHP < 8.5? And stacks < scalingo-26?
There was a problem hiding this comment.
That's a great question, I hadn't thought about it.
I tried to compile PHP 8.2.31 on scalingo-22 with this with-tidy flag and it seems to work. During the configure step, I can see:
checking for TIDY support... yes
checking for tidyOptGetDoc in -ltidy... yes
checking for tidyReleaseDate in -ltidy... yes
If I remove this flag, there is:
checking for TIDY support... no
Hence I think it's ok for all PHP versions on all supported stacks. WDYT?
There was a problem hiding this comment.
After some internal discussions, I eventually dropped it
| @@ -32,9 +32,12 @@ source "../conf/versions.sh" | |||
| # | |||
| skip=( "imap" "amqp" "igbinary" ) | |||
There was a problem hiding this comment.
Actually, amqp is now supported with PHP 8.5.x (https://doc.scalingo.com/changelog#changelog-buildpacks-php-amqp-ext-2-2-0), so (my bad, actually):
| skip=( "imap" "amqp" "igbinary" ) | |
| skip=( "imap" "igbinary" ) |
| php::pkg::download_extract "${zlib_url}" "${tempdir}" "${tempdir}" | ||
| php::pkg::zlib::compile "${tempdir}/zlib-${zlib_version}" "${zlib_dir}" | ||
|
|
||
| echo "-----> Installing libtidy from APT repository" |
There was a problem hiding this comment.
question: same as above, is this OK/required for stacks < scalingo-26?
There was a problem hiding this comment.
This is required for the with-tidy flag to work. It works on scalingo-22 with PHP 8.2.31.
There was a problem hiding this comment.
After some internal discussions, I eventually dropped it
There was a problem hiding this comment.
praise: thanks for adding the comments 👍
374120c to
2316e58
Compare
6c83b56 to
eb12c53
Compare
eb12c53 to
576eab6
Compare
Frzk
left a comment
There was a problem hiding this comment.
praise: thanks for all the hard work, adding comments where necessary, and cleaning things <333
| @@ -1,22 +1,25 @@ | |||
| #!/bin/bash | |||
| # | |||
| # Compile the Tidy PHP extension (https://www.php.net/manual/en/book.tidy.php). The outcome is a .so file that is uploaded to the object storage. This file is downloaded during the deployment if a customer enables the Tidy extension. | |||
|
|
||
| cd build/cmake | ||
| # The flag `-DCMAKE_POLICY_VERSION_MINIMUM=3.5` is mandatory for the call to CMake to succeed with versions available in scalingo-26 and more recent. | ||
| cmake ../.. -DCMAKE_INSTALL_PREFIX=/app/vendor/libtidy -DTIDY_COMPAT_HEADERS=true -DCMAKE_POLICY_VERSION_MINIMUM=3.5 |
There was a problem hiding this comment.
The flag -DCMAKE_POLICY_VERSION_MINIMUM=3.5 is mandatory starting with scalingo-26, but it works whatever the stack.
It is mandatory or we face the error:
CMake Error at CMakeLists.txt:20 (cmake_minimum_required):
Compatibility with CMake < 3.5 has been removed from CMake.
Update the VERSION argument <min> value. Or, use the <min>...<max> syntax
to tell CMake that the project requires at least <min> but has been updated
to work with policies introduced by <max> or earlier.
Or, add -DCMAKE_POLICY_VERSION_MINIMUM=3.5 to try configuring anyway.
No description provided.