-
Notifications
You must be signed in to change notification settings - Fork 7.9k
README: Fix non-existent iconv
in macOS instruction
#19475
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
base: master
Are you sure you want to change the base?
Conversation
Thanks, will merge after confirmation that the updated docs do indeed allow for build PHP |
Before installing libiconv, After installing libiconv, is still failed. Apparently, |
Okay, do you want to add that part to the documentation? |
@DanielEScherzer Sure! I believe that would require splitting the "configure" step by platform, like we do for the dependency installation step. Perhaps we can avoid this by fixing the discovery issue instead? I know very little about autoconf and m4, but the following change in - for i in $PHP_ICONV /usr/local /usr; do
+ for i in $PHP_ICONV /usr/local /opt/homebrew/opt/libiconv /usr; do I've amended the PR to include this. |
4e77bfd
to
1c21559
Compare
Unfortunately I also don't know too much about autoconf and m4, @petk would you mind reviewing? |
Little subtlety, because you can possibly have two different installations of homebrew, usually the one for rosetta is in /usr/local so in that case you would still need to set --with-iconv path explicitly with that order if you want for native arm64. |
./configure --enable-all --enable-debug --enable-opcache --with-iconv=$(brew --prefix libiconv) --enable-zend-test this is my shell to call |
Follows-up 22e444c (phpGH-18670). There is currently no formula called "iconv". [1][2] ``` $ brew install iconv Warning: No available formula with the name "iconv". Did you mean icon or cconv? ``` There package is called "libiconv". [3] Once installed, `./configure` still fails due to a discovery issue. ``` $ ./configure --enable-debug … checking for libiconv... no configure: error: Please specify the install prefix of iconv with --with-iconv=<DIR> ``` In 2020, as part of the switch from Intel to ARM, Homebrew adopted /opt as the standard location instead of /usr/local. [4][5] Rather than complicating the README with a mandatory `--with-iconv` path (or --without-iconv) for macOS users, improve the discovery to support Homebrew's new location. [1]: https://brew.sh/ [2]: https://github.com/Homebrew/homebrew-core/. [3]: https://formulae.brew.sh/formula/libiconv [4]: https://apple.stackexchange.com/a/437622/33762 [5]: https://docs.brew.sh/FAQ#why-is-the-default-installation-prefix-opthomebrew-on-apple-silicon
08f91e3
to
9c1fa7d
Compare
php_brew_prefix=$($BREW --prefix 2> /dev/null) | ||
fi | ||
|
||
for i in $PHP_ICONV $php_brew_prefix/opt/libiconv /usr/local /usr; do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't great, but I'm trying to keep the patch simple and follow existing patterns.
Usually PHP_ICONV=yes
so it creates a dummy entry that is naturally skipped (by virtue of there being no yes/
subdirectory). Likewise, $php_brew_prefix
will check either /opt/homebrew/opt/libiconv
or /usr/local/opt/libiconv
or no/opt/libiconv
where the latter is naturally skipped.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, this now looks good to me, I'll plan to merge this in a day or two if there are no objections
php_brew_prefix=$($BREW --prefix 2> /dev/null) | ||
fi | ||
|
||
for i in $PHP_ICONV $php_brew_prefix/opt/libiconv /usr/local /usr; do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, this now looks good to me, I'll plan to merge this in a day or two if there are no objections
Follows-up 22e444c (GH-18670). There is currently no formula called "iconv" at https://brew.sh/ or https://github.com/Homebrew/homebrew-core/.
There is one called
libiconv
, however.https://formulae.brew.sh/formula/libiconv