Skip to content

Improvement for bug #78360 - also try host name #4488

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Improvement for bug #78360 - also try host name #4488

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions ext/gmp/config.m4
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,13 @@ PHP_ARG_WITH([gmp],

if test "$PHP_GMP" != "no"; then

AC_CANONICAL_HOST
MACHINE_INCLUDES=$($CC -dumpmachine)

for i in $PHP_GMP /usr/local /usr; do
test -f $i/include/gmp.h && GMP_DIR=$i && break
test -f $i/include/$MACHINE_INCLUDES/gmp.h && GMP_DIR=$i && break
test -f $i/include/$host/gmp.h && GMP_DIR=$i && break
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this really addresses the root issue. The problem is that we're trying to look for gmp.h in a specific location, even though gmp.h is already on the default include path. We should instead be checking whether the compiler sees the gmp.h header, e.g. by using AC_LINK_IFELSE.

Copy link
Member

Choose a reason for hiding this comment

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

Since we're using pkg-config for several other extensions as well, shouldn't we use it for ext/gmp too?

Copy link
Member

Choose a reason for hiding this comment

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

GMP does not support pkg-config, unfortunately.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree that it is better to let the compiler figure out if it can see the header. My autoconf-fu just wasn't strong enough to figure out how to do that right, but I'll take another look. Shouldn't it be AC_COMPILE_IFELSE though? If we're looking for header, linking is not necessary I think?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe @petk also has ideas here on how to do this nicely.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, the -print-multiarch seems to be the name of this directory, yes. How about then another sed? Unless this unknown is also output by PHP's scripts then the $host_vendor might be unknown.

dnl comment here
MACHINE_INCLUDES=$($CC -dumpmachine | $SED "s/-$host_vendor-/-/" | $SED 's/-unknown//')

Providing the --host option to be able to build with clang is from user point of view a bit hard to figure out. I would like to be able to build php using clang with only something like this:

./configure CC=clang

and let those m4 files figure out what other things are needed for clang specifics. (in ideal case scenario of course).

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the mention, @petk. Always happy to be involved.

If you want to handle gcc and clang at the same time, you can simply do this:

-  MACHINE_INCLUDES=$($CC -dumpmachine)
+  MACHINE_INCLUDES=$($CC -print-multiarch 2> /dev/null || ($CC -dumpmachine | sed 's/-pc//'))

That said, the detection code in ext/gmp/config.m4 needs an overhaul. AC_CHECK_HEADERS works well, but doesn't provide the path. I'm hoping there is a macro we can use for that, or even write one.

Copy link
Member

Choose a reason for hiding this comment

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

I tried to find a way to do it in autoconf and I couldn't find the proper magic... AC_COMPILE_IFELSE and friends use current compiler settings and there seems to be no good way to see where the include comes from to determine the right GMP_DIR. I checked all other modules and most of them do the same - enumerate directories manually. So I guess we need somebody who understands autoconf better to take a look.

I think there are two different cases here: Manually specified GMP_DIR and use of system libgmp. If system libgmp is used, then both the include path and the lib path should already have the GMP headers / library and actually figuring out what the directories are shouldn't be necessary.

It might be easiest to just split these cases: If a path is specified, check for gmp.h as we do now. If it itsn't then use AC_LINK_IFELSE without any extra -I or -L to ensure both the library and header is found, and add the library without an additional libpath.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@hughmcmaster clang add -unknown, not -pc, at least on Travis system. Maybe on some other system it adds -pc too. That's the problem - it's not predictable, really.

It might be easiest to just split these cases

Maybe this is the best way, really.

Copy link
Contributor

Choose a reason for hiding this comment

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

If this is urgent, you can filter out the -pc or -unknown part of the string with ($CC -dumpmachine | cut -d '-' -f 1,3-).

I've sent a patch to add pkg-config support to upstream gmp, but it's still going to take time for a new version to appear with this.

done

if test -z "$GMP_DIR"; then
Expand Down