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

Use gnu-libiconv on alpine only for php iconv extension #1264

Merged
merged 1 commit into from
Mar 10, 2022

Conversation

msierks-pcln
Copy link
Contributor

@msierks-pcln msierks-pcln commented Mar 10, 2022

This is likely a controversial change. One issue we have with the Alpine images is the lack of translit support for php iconv extension. This will compile against gnu-libiconv instead of musl's implementation.

Also had to disable compiling the extension as shared object because we're unable to build the extension on PHP 8.0 and greater. Since compiling as shared object was added as a workaround to musl's iconv, I reverted that change across the board.

Open to feedback on this change.

Copy link
Member

@tianon tianon left a comment

Choose a reason for hiding this comment

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

After looking over the details you've provided (and testing way too many combinations locally, which led to lots of cussing about autoconf) and then finding https://www.php.net/manual/en/intro.iconv.php, which includes this gem:

Note that the iconv function on some systems may not work as you expect. In such case, it'd be a good idea to install the » GNU libiconv library. It will most likely end up with more consistent results.

IMO, that's a pretty reasonable "upstream recommendation" for us to just switch unilaterally. I think there's a lot of folks over on #240 that would really enjoy seeing this finally "fixed" in some way. 😄

Just a few minor stylistic nits (that I'm happy to make myself if you'd prefer). 👍

Dockerfile-linux.template Outdated Show resolved Hide resolved
Dockerfile-linux.template Outdated Show resolved Hide resolved
Dockerfile-linux.template Outdated Show resolved Hide resolved
@msierks-pcln
Copy link
Contributor Author

After looking over the details you've provided (and testing way too many combinations locally, which led to lots of cussing about autoconf) and then finding https://www.php.net/manual/en/intro.iconv.php, which includes this gem:

Note that the iconv function on some systems may not work as you expect. In such case, it'd be a good idea to install the » GNU libiconv library. It will most likely end up with more consistent results.

Interesting, I missed that little tidbit in the docs.

IMO, that's a pretty reasonable "upstream recommendation" for us to just switch unilaterally. I think there's a lot of folks over on #240 that would really enjoy seeing this finally "fixed" in some way. smile

That's great to hear, was a little unsure about this one.

Just a few minor stylistic nits (that I'm happy to make myself if you'd prefer). +1

Thanks, I made all the requested changes which are great. 😄

Also did some further testing, which has all been good so far. Please let me know if there is anything else.

@yosifkit yosifkit merged commit 399ed12 into docker-library:master Mar 10, 2022
@tianon tianon mentioned this pull request Mar 10, 2022
docker-library-bot added a commit to docker-library-bot/official-images that referenced this pull request Mar 10, 2022
Changes:

- docker-library/php@399ed12: Merge pull request docker-library/php#1264 from msierks-pcln/alpine-gnu-libiconv
- docker-library/php@b62fa33: Use gnu-libiconv for php iconv extension on alpine
- docker-library/php@7fcdbf3: Merge pull request docker-library/php#1262 from msierks-pcln/shared-iconv
- docker-library/php@85143a1: Update iconv extension to be shared, so it can be disabled if necessary
@jdreesen jdreesen mentioned this pull request Mar 15, 2022
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.

3 participants