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

Switch from curl-config to pkg-config for curl extension #2694

Closed
wants to merge 1 commit into from

Conversation

remicollet
Copy link
Member

@remicollet remicollet commented Aug 18, 2017

First attemp to fix multiarch support (#74125) for curl
introduce some debian specificity (dpkg command)
so is not suitable for other environmant.

This is mostly related to a broken "curl-config" config on debian
which doesn't provide the correct build options, while pkg-config
works as expected.

This new attemp rely on pkg-config output instead.

@remicollet
Copy link
Member Author

This PR is open for discussion.

ping @oerdnj as this is debian related.

@oerdnj
Copy link
Contributor

oerdnj commented Aug 18, 2017

Anything that reduces the number of patches we carry for M-A is welcome.

@remicollet
Copy link
Member Author

@oerdnj real question is: this PR revert a debian specific patch, and try another implementation (pkg-config), so does it still work on debian ?

First attemp to fix multiarch support (#74125) for curl
introduce some debian specificity (dpkg command)
so is not suitable for other environmant.

This is mostly related to a broken "curl-config" config on debian
which doesn't provide the correct build options, while pkg-config
works as expected.

This new attemp rely on pkg-config output instead.

Notice: this make pkg-config a hard dependency.
Is there system without pkg-config ?
@oerdnj
Copy link
Contributor

oerdnj commented Aug 18, 2017

I am reasonably certain it should work, but I can try to add this patch to next build and we'll see.

@remicollet
Copy link
Member Author

Changed to keep a fallback on curl-config (is some system don't have pkg-config)

@@ -6,75 +6,97 @@ PHP_ARG_WITH(curl, for cURL support,
[ --with-curl[=DIR] Include cURL support])

if test "$PHP_CURL" != "no"; then
if test -r $PHP_CURL/include/curl/easy.h; then
CURL_DIR=$PHP_CURL
Copy link
Contributor

Choose a reason for hiding this comment

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

should we use pkg-config even if the path is specified explicitly? I would expect the files in <somepath>/include/curl/easy.h to be used regardless of whether pkg-config is available, when I compile PHP with --with-curl=<somepath>

Copy link
Member Author

@remicollet remicollet Aug 21, 2017

Choose a reason for hiding this comment

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

should we use pkg-config even if the path is specified explicitly?

Yes, this is what is done, using libcurl.pc in specified directory

 elif test -r $PHP_CURL/lib/pkgconfig/libcurl.pc; then 

Which should (obviously) provides needed build options to use headers from this directory.

@remicollet
Copy link
Member Author

Notice we have 2 way to specify PATH:

 ./configure --with-curl=<installprefix>

Or

 PKG_CONFIG_PATH=<installprefix>/lib/pkgconfig ./configure --with-curl

IMHO, the second should be preferred, (first still works for compatibility)

@remicollet
Copy link
Member Author

Merged

@remicollet remicollet closed this Aug 25, 2017
@remicollet remicollet deleted the issue-curl-pkgconfig branch August 25, 2017 15:02
charlesportwoodii added a commit to charlesportwoodii/php-fpm-build that referenced this pull request Sep 28, 2017
…hp/php-src#2694

php/php-src#2694 tries to automatically find and use pkg-config/libcurl.pc to determine
the compilation flags. This is problematic when using a custom cURL binary that itself
used pkg-config with openssl. pkg-config does not correctly report the necessary compile
flags for cURL's config.m4.

PHP still supports the old curl-config option, which we can fall back upon.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants