RFR: 8148302: Initial Bringup of Android in mobile/dev

[Magnus Ihse Bursie]

On 2016-01-27 17:15, Bob Vandette wrote:
>> On Jan 27, 2016, at 4:56 AM, Magnus Ihse Bursie <magnus.ihse.bursie at 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 ).
> Thanks for catching that.  Fixed.
>
>> * 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.)
> Fixed.
>> * In CompileJavaModules.gmk, why do you remove the exclude for BUILD_HEADLESS_ONLY? This change affects all platforms, not only android.
> I don’t believe BUILD_HEADLESS_ONLY works for any platform without this fix.  Although I specify —disable-headful and set BUILD_HEADLESS_ONLY,
> the desktop module is still built and fails compiling several files due to these missing classes.
>
> * For target java.desktop:
> /export/users/bobv/mobile-dev/jdk/src/java.desktop/share/classes/sun/awt/www/content/audio/aiff.java:34: error: package sun.applet does not exist
> import sun.applet.AppletAudioClip;
>                   ^
> /export/users/bobv/mobile-dev/jdk/src/java.desktop/share/classes/sun/awt/www/content/audio/basic.java:34: error: package sun.applet does not exist
> import sun.applet.AppletAudioClip;
>
> /export/users/bobv/mobile-dev/jdk/src/java.desktop/share/classes/sun/awt/www/content/audio/wav.java:34: error: package sun.applet does not exist
> import sun.applet.AppletAudioClip;
>                   ^
> /export/users/bobv/mobile-dev/jdk/src/java.desktop/share/classes/sun/awt/www/content/audio/x_aiff.java:34: error: package sun.applet does not exist
> import sun.applet.AppletAudioClip;
>                   ^
> /export/users/bobv/mobile-dev/jdk/src/java.desktop/share/classes/sun/awt/www/content/audio/x_wav.java:34: error: package sun.applet does not exist
> import sun.applet.AppletAudioClip;
>                   ^
> /export/users/bobv/mobile-dev/jdk/src/java.desktop/share/classes/sun/font/SunFontManager.java:2843: error: package sun.applet does not exist
>                              return sm instanceof sun.applet.AppletSecurity;
>
>> * 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.
> I removed this change.  It’s no longer necessary.
>
> The change that most likely fixed this is the setting of BUILD_HEADLESS_ONLY.
>
> Shouldn’t BUILD_HEADLESS_ONLY be set when —disable-headful is set to true or maybe the uses of BUILD_HEADLESS_ONLY
> should be changed to SUPPORT_HEADFUL?

Yeah, the headless stuff is quite messy. :-( We've never really gotten 
around to cleaning it up. It seems you are right, we have a lot of tests 
for BUILD_HEADLESS_ONLY but never sets it (!). That does not make sense.

I think part of the confusion here is the lack of good terms. While 
"headless" is an accepted term, it's not really of any use to use since 
we *always* build the "headless" libraries. So the only thing that we 
can turn on and off is the "headful" form. But that's not really an 
accepted  term. And adding adjectives like "only" does not do wonders 
for good names, even if it's technically correct. As for defining 
BUILD_HEADLESS_ONLY, we really try to avoid making a difference just by 
checking for lack of definition, instead we assign "true" or "false" to 
a variable. I think this lack of a clear good name to use to select that 
which is relevant for us has held back an overhaul of this area.

In short, if you have a good suggestion of a variable name to use, I'd 
be more than happy to clean this mess up. :-)

/Magnus





>
>
>> * 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.
> Ok.
>> - 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.
> Yes, I was trying to model this after the freetype library.
>
> Ok, I’ll make that change.
>
>
>> 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!
> They are also removed from the VM makefiles.
>
> I’m happy that we don’t need this anymore.  This happened when we started generating devkits from the Android NDK.
> The NDK toolchain generation script generates toolchains that handle the ldscripts for us.
>
> Thanks for the review,
> Bob.
>
>> /Magnus