RFR: 8145132: Initial updates for ios x86_64 mobile/dev builds.

Thu Dec 17 16:09:27 UTC 2015

On 2015-12-17 14:19, Gary Adams wrote:
> I've revised the original webrev based on some feedback received.
>    - reverted white space only changes
>    - proper copyrights on the new files
>    - some hotspot files contained previously removed code
>
>   Webrev; http://cr.openjdk.java.net/~gadams/8145132/webrev.01/
>
> Planning to push this first batch tomorrow.

Hi Gary,

There seems to be multiple merge/diff errors in this patch. I'm seeing 
several location where this patch contains changes that is part of 
other, recently pushed change sets. This makes it hard to fully 
understand what changes you are contributing in this patch, to speak 
nothing of the merge problems that are likely to arise if this patch 
were pushed as it is.

I found several other issues as well. I'm sorry I have not been able to 
review this code before. It's a quite massive patch. If you want to 
commit the patch as-is in the mobile/dev forest and then fix my comments 
later before pushing further to mobile/jdk9, it's ok for me. I 
understand that it's tricky to manage a patch of this size. (But I think 
it's better to fix issues now, if you ask me...)

I'll start with something that you and Erik already has discussed: how 
to express tests for logic that is common to ios and macosx. There are 
places in the patch right now that I'm still not happy with.

First of all, I don't think you need to be shy of testing for macosx or 
ios. It's a bit more to write, but it's very clear to the reader, and 
code is read more often than written.

Second, I see you introduce a OPENJDK_TARGET_OS_ENV=macosx for ios. It's 
a bit strange, since it changes the meaning of the OS_ENV (previously it 
was only used to differentiate between cygwin and msys in Windows), but 
I think it could be a reasonable modification. This means that you can 
test if OPENJDK_TARGET_OS_ENV == macosx to cover both the case of macosx 
and ios. Let's just assume that you need to know what you're doing when 
looking at OS_ENV. So this could be an alternative to a lot of "if 
target == macosx or target == ios", if you don't want to type that 
everywhere. For android it seems less of a point of setting OS_ENV. Or 
do you think you could unify android/linux code by this?

Third, I'm not really fond of the TOOLCHAIN_NAME variable. The concept 
is fine, but the variable name is not (I know you didn't invent this).  
We have a TOOLCHAIN_DESCRIPTION and a TOOLCHAIN_NAME sounds like just a 
variant of that. Perhaps resusing the fluffy and unspecified ENV and 
call it TOOLCHAIN_ENV instead? I thought of TOOLCHAIN_VENDOR, but then 
again I'd assume that it'd be "apple", and I think we gain clarity by 
calling Xcode "xcode" and not "apple".

So, to the specifics:
basics.m4: It's not really true that this is build os rather than target 
os. In fact, the whole block is a mix of target and build dependent 
stuff. For instance, xattr depends on build platform but dsymutil 
depends on target os. I suggest you change to target == macosx || ios.

boot-jdk.m4: At the end looks like merge error.

flags.m4: Why delete CPPFLAGS for SYSROOTS?

I think that if you set only LEGACY_EXTRA_CFLAGS and not EXTRA_CFLAGS, 
you will only pass this to Hotspot and not the jdk libraries. But 
honestly, the flag handling is mysterious even to me so I can't say for 
sure. But you might want to double-check this.

BUILD_CC check looks like merge error.

lib-bundled.m4:
Out choice to use system zlib has nothing to do with xcode. It's a 
target os decision.

lib-freetype.m4: OTOH, I'd say that this is indeed a build-os decision :-)

lib-std.m4: This test is only valid for gcc. We are currently not using 
gcc, only clang, for macosx builds. Are you really using gcc for ios 
builds? Otherwise, just leave it as it is.

spec.gmk.in:
Looks like mostly (only?) merge problems here. Or have you added something?

toolchain.m4: See discussion above on TOOLCHAIN_NAME. It looks like 
there are lot of merge errors here.

CompileJavaModules.gmk: Merge errrors galore. I can't really tell what 
are your changes in here.

Images.gmk: Care to elaborate? I don't understand anything.

Main.gmk, JavaCompilation.gmk, Modules.gmk: Merge errors. What is your 
changes in these files? I can't review this.

MakeBase.gmk: just wanted to let you know that I approve of the change 
to build os. :-) I'd appreciate if you could fix the whitespace mistake 
as well (space after the comma is missing).

SetupJavaCompilers.gmk: merge errors? For the GENERATE_JAVA5_BYTECODE, 
please use space after comma. (I know we didn't do that everywhere 
originally, but new code should adhere to the styleguide.)

jdk/make/CompileDemos.gmk: Merge erros. The new android ifdef should 
have proper whitespace (space after comma, the if block indented two 
spaces).

jdk/make/Import.gmk: Looks like the block after the two ifneq 
($(OPENJDK_TARGET_OS), ios) has not been indented. (Or is it a problem 
with the webrev?) Also, the android ifdef is weirdly indented, looks 
like you tried to cram it in with just a single space indent.

jdk/make/gensrc/GensrcCharsetMapping.gmk, 
jdk/make/gensrc/Gensrc-jdk.jdi.gmk, 
jdk/make/gendata/Gendata-java.base.gmk: indentation in if blocks is missing.

jdk/make/launcher/Launcher-java.base.gmk and other launcher make files: 
I thought you and Erik agreed to remove this?

jdk/make/lib/CoreLibraries.gmk: Why remove ARFLAGS? On macosx, we put 
the -framework options in LIBS rather than LDFLAGS (the latter is only 
used to change the behavior of the linker, not determine what to link 
with.)

jdk/make/lib/Awt2dLibraries.gmk:  LIBAWT_EXFILES += initIDs.c 
awt/image/cvutils/img_colors.c should not depend on build os. That's a 
target os decision. The same goes for the LIBSPLASHSCREEN_DIRS changes. 
Also, the whole file seems to be excluded on ios so why these changes at 
all? (And, if that ifdef really is correct, you need to indent the whole 
file as well.) And the same goes for -framework into LIBS not LDFLAGS 
(but only if it's really used on ios).

jdk/make/lib/Lib-java.base.gmk: I'm not sure what's happening here. Can 
you elaborate?

jdk/make/lib/Lib-java.prefs.gmk, jdk/make/lib/Lib-java.instrument.gmk: 
indentation seems wrong, it should be two spaces.

jdk/make/lib/Lib-jdk.jdwp.agent.gmk: Why the LIBDIR stuff?

jdk/make/lib/Lib-jdk.management.gmk, jdk/make/lib/Lib-jdk.sctp.gmk: 
Indentation, indentation...

jdk/make/lib/LibCommon.gmk: Why the strange order in the android case?

jdk/make/lib/SoundLibraries.gmk: indentation (last if block, the rest 
looks fine)

jdk/src/java.base/share/tools/jproject/ios/frameworks/*...: I'm not sure 
this is the correct location. We've never had a "share/tools" directory 
before in a module. They are needed to building the JavaLauncher static 
library on ios, right? I think they should reside with the source code, 
then, in jdk/src/java.base/ios/native/JavaLauncher. (but see below about 
the name).

jdk/src/java.base/ios/native/JavaLauncher: Directories should only use 
lower case. Normally, we prefix libraries with "lib" in the native 
directory, directories without "lib" are supposed to be programs. Now 
this is a static library and we haven't had many of these before, but 
libjli is static (or can be) and still have the lib prefix, so I'd argue 
that thisshould be jdk/src/java.base/ios/native/libjavalauncher instead.

make/lib/JavaLauncher.gmk: For consistency (and to clarify that this is 
libraries, not launchers), it should be named JavaLauncherLibraries.gmk. 
The android part creates a jar. This should not be done in the native 
phase, but in the Java phase. OCLDVK_OUTPUTDIR  is defined but not used. 
It's a bit sad to commit new code with WARNINGS_AS_ERRORS_clang := 
false. :-( Can't you fix the warnings instead?

---

This was what I could find right now. I have not looked at hotspot build 
changes, nor any source code changes.

/Magnus