Hello Please take a look at updated webrev - http://cr.openjdk.java.net/~vkempik/8248495/webrev.02/ <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. 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
