Skip to content

Conversation

nikic
Copy link
Member

@nikic nikic commented Aug 29, 2019

This is an alternative to #4488. It splits the default and custom path case. If default is used, we assuming that the header must be on the include path and the library in the libdir, so we don't add anything more than that. For the custom path case we do what we previously did.

I also added a small hack for ldap, because it has some very complex detection logic that I don't want to touch.

cc @smalyshev @petk

@@ -79,7 +79,8 @@ if test "$PHP_LDAP" != "no"; then
LDAP_PTHREAD=
fi

MACHINE_INCLUDES=$($CC -dumpmachine)
dnl -pc removal is a hack for clang
MACHINE_INCLUDES=$($CC -dumpmachine | sed 's/-pc//')
Copy link
Member Author

Choose a reason for hiding this comment

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

Seeing how most of the code below is 19 years old (https://github.com/php/php-src/blame/master/ext/ldap/config.m4#L85), I really wonder how much of it is really necessary. Searching for libssldap50 shows last mentions around ~2005...

Copy link
Member

Choose a reason for hiding this comment

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

Regarding the ldap extension this is basically done to resolve a similar case as the one with gmp and there will be the same issue then as described in the comments at #4488 - there might be also -unknown used as a $host_vendor. So this sed solution above won't solve all possible cases.

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, it would be better to fix this properly as well. However, the library detection code here is very complex. I've submitted #4658 to clean out some parts.

Generally what is being done here is very suspicious, I don't think we do actual path-based checking for library files anywhere else. I'm thinking that this should be migrated to use something like PHP_CHECK_LIB.

@petk
Copy link
Member

petk commented Aug 31, 2019

Very neat. Works ok. 👍

Co-Authored-By: Peter Kokot <peterkokot@gmail.com>
@nikic
Copy link
Member Author

nikic commented Sep 6, 2019

Merged as 34f4ad6.

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.

2 participants