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
