Here’s the updated webrev with updates based on your feedback. http://cr.openjdk.java.net/~bobv/8148302/webrev.01
I implemented all of the changes suggested. For the X11 issue, I removed my changes and replaced BUILD_HEADLESS_ONLY with SUPPORT_HEADFUL. This allows a headless only build to be fully controlled by —disable-headful. This shouldn’t impact our ARMv8 build since we set both —disable-headful and BUILD_HEADLESS_ONLY. Thanks, Bob. > On Jan 27, 2016, at 4:56 AM, Magnus Ihse Bursie > <magnus.ihse.bur...@oracle.com> wrote: > > On 2016-01-26 20:50, Bob Vandette wrote: >> Please review the changes for two improvements for the Mobile Dev Forest. >> >> With these changes, the mobile/dev forest can successfully build x86 and ARM >> Android >> JDK 9 binaries. The ARM binaries use the Zero ARM interpreter while the x86 >> implementation uses standard x86 Hotspot VMs. >> >> The webrev also includes 1 fix for iOS BSD builds in >> hotspot/make/bsd/makefiles/vm.make. >> >> JDK-8148304 Mobile: iOS builds fail on assembler files >> https://bugs.openjdk.java.net/browse/JDK-8148304 >> >> JDK-8148302 Mobile: Initial bring of Android x86 and ARM >> https://bugs.openjdk.java.net/browse/JDK-8148302 >> >> Here’s the webrev: >> >> http://cr.openjdk.java.net/~bobv/8148302/webrev > > Hi Bob, > > Some comments: > > * In jni_util_md.c, you have accidentally lost a trailing ). > > * In spec.gmk.in, please use ":=" instead of "=". The former is assignment, > the latter is used for macro expansion. (As a rule of thumb, := should always > be used unless you have explicit reasons.) > > * In CompileJavaModules.gmk, why do you remove the exclude for > BUILD_HEADLESS_ONLY? This change affects all platforms, not only android. > > * In lib-x11.m4, I *really* don't like adding X11 paths when we should not > have X11! (And it's even worse since you do not check for Android but do so > for all platforms.) What files are needed, and for what library? Can you not > fix this dependency by removing the #include statements in the source code > instead? Otherwise, I'd say that android *is* indeed dependent on X11, and > you need to express this properly. > > * In lib-ffi.m4 and Copy-java.base.gmk: I think this a bit problematic. I > believe what you're trying to achieve is similar to what we do for freetype, > where we "bundle" the library with the resulting product. I'd rather see this > libffi bundling based on that behavior. More concretely: > - The name of the variable should be like LIBFFI_BUNDLE_LIB_PATH, to clearly > show that this is used only when bundling the product, not when compiling it. > - Whether to bundle or not should be determined in lib-ffi.m4. The default > value should be "bundle if android+zero, otherwise not". If you want to, you > can add a --enable-libffi-bundling to override this. I won't require it, but > I generally find such things useful since you can test that bundling works > even without building for a specific target. The code in Copy-java.base.gmk > should then only test if this variable is present, compare with freetype in > Copy-java.desktop. Finally, the configure code must check for the case that > libffi bundling is enabled but no explicit libffi path is set (e.g. it is > autodetected). It's okay to abort with an error, but it must be detected. > > And finally, a note of appreciation. :-) I'm really happy to see the ugly > solution with the hard-coded linker scripts go away in flags.m4! > > /Magnus
