I sent this before but it seems it did not reach the mailing list..? Resending, apologies if this arrives twice.
--- > On 2020-07-01 11:45, Vladimir Kempik wrote: > Hello > > Please take a look at updated webrev - > http://cr.openjdk.java.net/~vkempik/8248495/webrev.02/ > > I decided to use AC_CHECK_HEADERS instead AC_CHECK_FILE as it doesn’t work in > cross-compilation scenario. > > libffi binary is located in absolutely standard location, so > -lffi was enough. This looks much better. However, it is still not correct. if test "x[$]$1SYSROOT" != "x"; then This looks some code you have copied from elsewhere, out of context. LIB_SETUP_LIBFFI does not take any argument, so $1 will always be empty. The test you are looking for is just "x$SYSROOT". And finally, a stylistic issue: please do not include a trailing slash on paths. If we ever need to concatenate paths, we will always add a "/" anyway. It just looks bad, and at times double slashes can cause trouble. /Magnus > > Thanks, Vladimir > >> 30 июня 2020 г., в 23:09, Magnus Ihse Bursie <magnus.ihse.bur...@oracle.com> >> написал(а): >> >> >>> On 2020-06-30 21:08, Vladimir Kempik wrote: >>> Hello >>> >>> I agree modding hpp files is a bad idea >>> >>> Thanks for idea with setting LIBFFI_CFLAGS >>> >>> here is updated webrev: >>> http://cr.openjdk.java.net/~vkempik/8248495/webrev.01/ >> I still think you are doing this too complicated, and the wrong way around. >>> >>> AC_CHECK_HEADERS ignored CFLAGS for some reason, so modding header_name for >>> it was still needed. >> >> No, AC_CHECK_HEADERS does not work that way. It knows nothing about our >> internal variables. How could it? >> >> First of all, I still think you should let PKG_CHECK_MODULES do its magic >> first. If that fails, you can try compiling with AC_CHECK_HEADERS([ffi.h]), >> just as the code currently does. >> >> Only if this fails, your workaround should kick in, before giving up >> completely. At this point, you should check if >> ${SYSROOT}/usr/include/ffi/ffi/ffi.h exists. If it does, you should set >> LIBFFI_CFLAGS := -I${SYSROOT}/usr/include/ffi/ffi >> >> and you will not need the AC_CHECK_HEADERS, since you know the ffi.h file is >> there, and there is a AC_LINK_IFELSE at the end to verify that everything >> works. You can even skip the platform checks, since this will apply to all >> configurations where the header file is in this odd place relative to the >> sysroot. (But please save a comment about where you have spotted it.) >> >> However, you are not done yet. Your patch do not address the whereabouts of >> the library, only the include file. I assume it might too be stored in an >> odd location? >> >> I see that we do not follow the best-practice of separating LDFLAGS and LIBS >> here, so if you need to point to a non-standard location for the library, >> you have to do like this example: >> >> LIBFFI_LIBS="-L${with_libffi}/lib -lffi" >> >> Ideally, this should be explit out to an LIBFFI_LDFLAGS, but that's a change >> for another day, since it required changes in many places. >> >> /Magnus >> >> >> >>> This special case only applies to macos/clang when sysroot is set and no >>> libffi configure options is used. (which is default case) >>> >>> Thanks, Vladimir >>> >>> >>>> 30 июня 2020 г., в 19:46, Magnus Ihse Bursie >>>> <magnus.ihse.bur...@oracle.com> написал(а): >>>> >>>> Vladimir, >>>> >>>> This looks like it can break in other situation than your specific case. >>>> >>>> It sounds like you should set LIBFFI_CFLAGS= to -I<path to your ffi >>>> installation>, such that "<path to your ffi installation>/ffi.h" exists. >>>> In particular, the change of include path in globalDefinitions_zero.hpp >>>> looks bad. >>>> >>>> /Magnus >>>> >>>>> On 2020-06-30 15:33, Vladimir Kempik wrote: >>>>> Hello >>>>> >>>>> Please review this fix for zero vm building on macos. >>>>> >>>>> The issue comes from the libffi, it’s headers are located inside >>>>> usr/include/ffi/ folder in Macos.sdk, so it can’t be found by configure >>>>> script. >>>>> >>>>> If one wants to use system’s libffi and pass path to libffi via configure >>>>> argument as --with-libffi-include=/usr/include/ffi, then it won’t be >>>>> found by configure because clang will look exactly in /usr/include/ffi, >>>>> but not in macos.sdk >>>>> The system, at least on 10.15 doesn’t have /usr/includes at all. >>>>> >>>>> This patch makes jdk to look for ffi/ffi.h header in case of Macos/clang >>>>> and no --with-libffi-include argument. >>>>> >>>>> However there is one issue with this patch, if --with-libffi-include >>>>> passed then c++ code will still try to include <ffi/ffi.h> >>>>> >>>>> I’m not sure which way is the best for such rare case. it could be >>>>> possible to define include filename in configure and pass it via -D and >>>>> CFLAGS to c++ code. >>>>> >>>>> >>>>> The webrev - http://cr.openjdk.java.net/~vkempik/8248495/webrev.00/ >>>>> >>>>> The bug - https://bugs.openjdk.java.net/browse/JDK-8248495 >>>>> >>>>> Thanks, Vladimir
