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

ext/gd: Use pkg-config to detect the availability of freetype2 #3632

Closed
wants to merge 1 commit into
base: master
from

Conversation

5 participants
@eli-schwartz
Copy link
Contributor

eli-schwartz commented Oct 22, 2018

The latest version of freetype2 does not install freetype-config by default, but pkg-config support has been there for approximately 15 years. In order to reliably detect freetype2, pkg-config must be used.

See:
https://savannah.nongnu.org/bugs/?53093
https://bugs.php.net/bug.php?id=76324

This PR contains my original changes which prompted #3630

ext/gd: Use pkg-config to detect the availability of freetype2
The latest version of freetype2 does not install freetype-config by
default, but pkg-config support has been there for approximately 15
years. In order to reliably detect freetype2, pkg-config *must* be used.

See:
https://savannah.nongnu.org/bugs/?53093
https://bugs.php.net/bug.php?id=76324
PHP_ARG_WITH(freetype-dir, for FreeType 2,
[ --with-freetype-dir[=DIR] GD: Set the path to FreeType 2 install prefix], no, no)
PHP_ARG_ENABLE(freetype, for FreeType 2,
[ --enable-freetype GD: Enable FreeType 2 support], no, no)

This comment has been minimized.

@weltling

weltling Oct 30, 2018

Contributor

No need to change the switch name. It's related to an external dependency, so it's definitely a with switch.

Thanks.

Show resolved Hide resolved ext/gd/config.m4
@nikic

This comment has been minimized.

Copy link
Member

nikic commented Oct 31, 2018

I agree with @eli-schwartz and would accept this PR as-is for master. We should not retain support for custom --with-* options. These are causing more and more issues as time goes by (regularly happens that libs get moved to target-specific directories and we don't keep up. Also more exotic systems don't work out of the box, ref #2697).

It might me useful to include a note in UPGRADING about the new way to specify a custom libfreetype2 (or other checks that will hopefully be migrated to pkg-config as well).

@eli-schwartz

This comment has been minimized.

Copy link
Contributor Author

eli-schwartz commented Nov 2, 2018

pkg-config in general is always nice to use properly, and some things already use pkg-config. I've actually played around a bit with using it more thoroughly, see https://github.com/eli-schwartz/php-src/commits/pkg-config-everywhere

This could be used as a followup to this PR.

@eli-schwartz

This comment has been minimized.

Copy link
Contributor Author

eli-schwartz commented Nov 2, 2018

It might me useful to include a note in UPGRADING about the new way to specify a custom libfreetype2 (or other checks that will hopefully be migrated to pkg-config as well).

Is this really necessary though? Part of the reason it's good to consistently use the standard macro, is because it even comes with useful ./configure --help output. Currently, with my patch applied, it mentions at the end:

Some influential environment variables:
  PKG_CONFIG  path to pkg-config utility
  PKG_CONFIG_PATH
              directories to add to pkg-config's search path
  PKG_CONFIG_LIBDIR
              path overriding pkg-config's built-in search path
[...]
  FREETYPE2_CFLAGS
              C compiler flags for FREETYPE2, overriding pkg-config
  FREETYPE2_LIBS
              linker flags for FREETYPE2, overriding pkg-config
[...]

And the actual error message the configure script uses when freetype cannot be found is even more elucidative:

Package requirements (freetype2) were not met:

$FREETYPE2_PKG_ERRORS

Consider adjusting the PKG_CONFIG_PATH environment variable if you
installed software in a non-standard prefix.

Alternatively, you may set the environment variables FREETYPE2_CFLAGS
and FREETYPE2_LIBS to avoid the need to call pkg-config.
See the pkg-config man page for more details.

It would be odd if a Unix system did not have pkg-config, but if it doesn't, then it instead emits the error:

The pkg-config script could not be found or is too old.  Make sure it
is in your PATH or set the PKG_CONFIG environment variable to the full
path to pkg-config.

Alternatively, you may set the environment variables FREETYPE2_CFLAGS
and FREETYPE2_LIBS to avoid the need to call pkg-config.
See the pkg-config man page for more details.

To get pkg-config, see <http://pkg-config.freedesktop.org/>.
See `config.log' for more details"
@nikic

This comment has been minimized.

Copy link
Member

nikic commented Nov 7, 2018

Those error messages do look quite informative. I've applied the PR as 2d03197 with tweaks to also distribute pkg.m4 with phpize and add a small note in UPGRADING regarding the changed name for the configure option.

Can you please also submit a PR for the work you did on porting pkg-config use in other extensions?

@nikic nikic closed this Nov 7, 2018

@cmb69

This comment has been minimized.

Copy link
Contributor

cmb69 commented Nov 7, 2018

salathe pushed a commit to salathe/phpdoc-en that referenced this pull request Nov 7, 2018

Christoph Michael Becker
@weltling

This comment has been minimized.

Copy link
Contributor

weltling commented Nov 7, 2018

If pkg-config is implemented, the old way to configure would serve compatibility on older systems, if needed. Clear, that pkg-config is a future oriented portable way.

My question in the other ticket was not answered unfortunately #3630 (comment), so not sure there were no other implications with other libs, if a custom lib needs to be linked.

Thanks.

@eli-schwartz

This comment has been minimized.

Copy link
Contributor Author

eli-schwartz commented Nov 7, 2018

I specified in your comment here:

pkg-config is the standard for interoperable, configurable dependency handling. Rather than using --with-foo-dir=/vendored/sysroot/ --with-bar-dir=/vendored/sysroot/ --with-baz-dir=/vendored/sysroot/, you can use PKG_CONFIG_PATH=/vendored/sysroot/lib/pkgconfig/ and pkg-config will automatically prefer the versions installed to the custom PKG_CONFIG_PATH, for all three.

@weltling

This comment has been minimized.

Copy link
Contributor

weltling commented Nov 7, 2018

@eli-schwartz yep, that's what the doc tells. What it doesnt is - in case like PKG_CONFIG_PATH=/path/to/one/lib:/path/to/another/lib:$PKG_CONFIG_PATH, would pkg-config try all the listed paths in the order? Usually it's more handier to use a separate prefix for every lib version, like /path/to/lib-1.2.3 and /path/to/lib-4.5.6, so then it is possible to quickly switch between versions. Otherwise, especially if there are some more different custom libs, one would be enforced to use same prefix for every permutation needed.

Thanks.

@eli-schwartz

This comment has been minimized.

Copy link
Contributor Author

eli-schwartz commented Nov 7, 2018

Huh?

Are you asking whether PKG_CONFIG_PATH is allowed to contain multiple (colon-separated) directories in the first place?

Because the docs say yes: https://linux.die.net/man/1/pkg-config or https://man.openbsd.org/pkg-config.1#PKG_CONFIG_PATH
And so does the ./configure --help text.

Are you asking whether it will only use the first directory listed? I'm not sure why you'd think that.

@nikic

This comment has been minimized.

Copy link
Member

nikic commented Nov 7, 2018

I think the question is whether, if a package is present in multiple directories in PKG_CONFIG_PATH, whether it will pick up the first one.

@weltling

This comment has been minimized.

Copy link
Contributor

weltling commented Nov 7, 2018

Are you asking whether it will only use the first directory listed? I'm not sure why you'd think that.

Logic were of course, that it does the usual path resolution and looks for the libraries according to the given path order. The world is not always as logic as one would want ;) I had no time to test it, thus asked. Thanks for explaining.

@weltling

This comment has been minimized.

Copy link
Contributor

weltling commented Nov 7, 2018

@nikic yeah, exactly. I guess it's answered now.

Thanks.

@eli-schwartz

This comment has been minimized.

Copy link
Contributor Author

eli-schwartz commented Nov 7, 2018

Okay, great. :)

Answer is that it just loops through each directory on the search path (that is to say, $PKG_CONFIG_PATH:$PKG_CONFIG_LIBDIR where $PKG_CONFIG_LIBDIR defaults to a compile-time search path) and returns the first pc file it finds.

@eli-schwartz eli-schwartz deleted the eli-schwartz:pkg-config branch Nov 7, 2018

@nikic

This comment has been minimized.

Copy link
Member

nikic commented Nov 7, 2018

Followup to fix travis config in 50e7e3a.

@petk

This comment has been minimized.

Copy link
Member

petk commented Nov 7, 2018

Maybe also updating the UPGRADING.INTERNALS file anyway should be done because removing/adding/refactoring a configure option is important change for automated builds for packages and similar...

eli-schwartz added a commit to eli-schwartz/php-src that referenced this pull request Nov 7, 2018

ext/gd: use --with instead of --enable
By convention it probably makes sense to stick with this even when
dropping the *-dir=DIR part.

Fix the travis script, which is either way using the wrong version, with
*-dir

See:
php#3632 (comment)
https://autotools.io/autoconf/arguments.html

eli-schwartz added a commit to eli-schwartz/php-src that referenced this pull request Nov 7, 2018

ext/gd: use --with instead of --enable
By convention it probably makes sense to stick with this even when
dropping the *-dir=DIR part.

Fix the travis script, which is either way using the wrong version, with
*-dir

See:
php#3632 (comment)
https://autotools.io/autoconf/arguments.html

svn2github pushed a commit to svn2github/phpdoc_en that referenced this pull request Nov 8, 2018

eli-schwartz added a commit to eli-schwartz/php-src that referenced this pull request Nov 9, 2018

ext/gd: use --with instead of --enable
By convention it probably makes sense to stick with this even when
dropping the *-dir=DIR part.

Fix the travis script, which is either way using the wrong version, with
*-dir

See:
php#3632 (comment)
https://autotools.io/autoconf/arguments.html

php-pulls pushed a commit that referenced this pull request Dec 26, 2018

ext/gd: use --with instead of --enable
By convention it probably makes sense to stick with this even when
dropping the *-dir=DIR part.

See:
#3632 (comment)
https://autotools.io/autoconf/arguments.html
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment