-
Notifications
You must be signed in to change notification settings - Fork 7.9k
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.
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
Conversation
This way if a compiler produces broken machine name, we can always override with --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 |
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.
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
.
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.
Since we're using pkg-config for several other extensions as well, shouldn't we use it for ext/gmp too?
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.
GMP does not support pkg-config, unfortunately.
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.
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?
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.
Maybe @petk also has ideas here on how to do this nicely.
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.
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).
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.
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.
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.
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.
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.
@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.
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.
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.
P.S: the target branch can be here something between PHP-7.2 and 7.4 I think since compiling with clang should be ideally supported. |
Merged alternative fix in #4646. |
This way if a compiler produces broken machine name, we can always
override with --host.