On 12/17/15 11:09, Magnus Ihse Bursie wrote:
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.
My bad. In getting the workspace to build I had diff'ed the files
against the latest jdk9-dev sources
and pulled in changes that were ahead of the recently cloned mobile/dev
repos.
The next merge will bring in those updates. mobile/dev is cloned from
jdk9-b94 and latest jdk9/dev
is at jdk9-9+b96.
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'd like to do any simple cleanups before the initial push. We know
there will be more pushes coming, so I'm not opposed to partial solutions
being put back at this time.
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.
I am uncomfortable with all the places platform specific changes need to
be made.
There isn't an easy way to express platform A is like platform B with
the following
differences.
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?
I thought previously we were told not to use OPENJDK_TARGET_OS_ENV and I
asked at that time for "documentation".
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".
I'd be glad to use a different name or value for the flag, just let me know.
This is a change we could defer til the mobile/dev to mobile/jdk9
integration.
At this time I'm mostly interested to know that we've used it in the
right places.
e.g. prior to discovery of boot-jdk and xcode we used BUILD_OS macosx,
after
that we use toolchain xcode check, for native flags and libraries we
still check TARGET_OS
for macosx or ios when not xcode specific.
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.
Will do.
boot-jdk.m4: At the end looks like merge error.
Will fix.
We had the big java fix in ejdk8u and it recently cam back in jdk9
recent builds.
flags.m4: Why delete CPPFLAGS for SYSROOTS?
SYSROOT does not belong on CPPFLAGS.
We needed the fix for the ios builds.
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.
Not sure about this comment. We have working ios and android builds from
the flags
that are currently being set. Several fingers over several years have
touched these
files, so the original intent may not be clear.
BUILD_CC check looks like merge error.
Will fix.
lib-bundled.m4:
Out choice to use system zlib has nothing to do with xcode. It's a
target os decision.
Will fix.
lib-freetype.m4: OTOH, I'd say that this is indeed a build-os decision
:-)
Will fix.
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.
Only changed it because it was flagged as
should be changed if we switched from xcode clang to
gcc based toolchain in the future.
Will revert/fix.
spec.gmk.in:
Looks like mostly (only?) merge problems here. Or have you added
something?
Will roll back the sjava related changes, but keep
the ":=" fix on mac osx version.
toolchain.m4: See discussion above on TOOLCHAIN_NAME. It looks like
there are lot of merge errors here.
Will recheck.
CompileJavaModules.gmk: Merge errrors galore. I can't really tell what
are your changes in here.
Will fix
Images.gmk: Care to elaborate? I don't understand anything.
ios builds do not build any command line executables.
It is the wrong target to build a dependency when the jlmage is
the result of the recipe.
# Use this file inside the image as target for make rule
-JIMAGE_TARGET_FILE := bin/java$(EXE_SUFFIX)
+JIMAGE_TARGET_FILE := lib/modules/bootmodules.jimage
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).
Fixed.
(Sure would be nice if there was tool to do the makefile whitespace
checking.)
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.)
Fixed.
I assume it's OK to fix the other missing space-after-comma.
jdk/make/CompileDemos.gmk: Merge erros. The new android ifdef should
have proper whitespace (space after comma, the if block indented two
spaces).
Fixed.
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.
Fixed.
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.
Fixed.
jdk/make/launcher/Launcher-java.base.gmk and other launcher make
files: I thought you and Erik agreed to remove this?
The addition of new configure options will be handled in subsequent pushes.
This is one of the places where new features need to be developed
to be ready for a mobile/jdk9 push, but should not block getting basic ios
functionality into the open repos.
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.)
We ran into conflicts with the use of ARFLAGS and AR_OUTPUT_OPTION in
NativeCompilation.gmk.
It does not appear that both could be set and build properly.
# Generating a static library, ie object file archive.
$$($1_TARGET): $$($1_EXPECTED_OBJS) $$($1_RES) $$($1_VARDEPS_FILE)
$(ECHO) $(LOG_INFO) "Archiving $$($1_STATIC_LIBRARY)"
$(call LogFailures, $$($1_OBJECT_DIR)/$$($1_SAFE_NAME)_link.log,
$$($1_SAFE_NAME)_link, \
$$($1_AR) $$($1_ARFLAGS) $(AR_OUT_OPTION)$$($1_TARGET)
$$($1_EXPECTED_OBJS) \
$$($1_RES))
There are also ARFLAGS references in hotspot/bsd/makefiles that may need
unification
when the hotspot new build is set up.
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).
Unfortunately our android developer retired, so I'm not totally up to
date on the selective files being excluded.
I'm assuming they did not compile on the target platform and were simply
excluded. The mobile platforms
are intend as headless compact2 only. I'm pretty sure the whole file
ios exclusion was done after some
number of attempts to get the library to compile.
This is a another place we expect to fix later with a proper working of
headless builds.
I'll fix the LIBS reference to framework before the initial push.
jdk/make/lib/Lib-java.base.gmk: I'm not sure what's happening here.
Can you elaborate?
On ios there is no libjli. The app container is not allowed to fork/exec
new processes.
That is why a separate JavaLauncher interface is provided to ease the
startup of a VM in the current process.
The other logic in the selection of symbol file to use is based on the
VM actually
being built. e.g. minimal, client, server vm. The static macosx build
only supported
server vm. We deplot mobile with minimal vm, but still maintain client
vm for testing purposes.
jdk/make/lib/Lib-java.prefs.gmk, jdk/make/lib/Lib-java.instrument.gmk:
indentation seems wrong, it should be two spaces.
Fixed.
jdk/make/lib/Lib-jdk.jdwp.agent.gmk: Why the LIBDIR stuff?
There is still a remnant of arch specific directories in the build system.
Here's the relevant setting from platform.m4
# This is the name of the cpu (but using i386 and amd64 instead of
# x86 and x86_64, respectively), preceeded by a /, to be used when
# locating libraries. On macosx, it's empty, though.
OPENJDK_TARGET_CPU_LIBDIR="/$OPENJDK_TARGET_CPU_LEGACY_LIB"
if test "x$OPENJDK_TARGET_OS" = xmacosx; then
OPENJDK_TARGET_CPU_LIBDIR=""
fi
AC_SUBST(OPENJDK_TARGET_CPU_LIBDIR)
Ideally, the build system would allow us to cross compile
multiple targets in a single pass.
macosx build system
ios-x86
ios-x86_64
ios-arm
ios-aarch64
then roll up universal static libraries with lipo.
Since we do not have that capability we build 4 separate bundles
and our users are combining 4 platform bundles and rolling their own
universal libraries.
jdk/make/lib/Lib-jdk.management.gmk, jdk/make/lib/Lib-jdk.sctp.gmk:
Indentation, indentation...
Fixed.
(Wish there was a tool that could fix the whitespace.)
jdk/make/lib/LibCommon.gmk: Why the strange order in the android case?
I read the android diff to mean use the "linux" native code when
TARGET_OS is android.
+ifeq ($(OPENJDK_TARGET_OS), android)
+FindSrcDirsForLib = \
+ $(call uniq, $(wildcard \
+ $(JDK_TOPDIR)/src/$(strip $1)/*linux*/native/lib$(strip $2) \
+ $(JDK_TOPDIR)/src/$(strip
$1)/$(OPENJDK_TARGET_OS_TYPE)/native/lib$(strip $2) \
+ $(JDK_TOPDIR)/src/$(strip $1)/share/native/lib$(strip $2)))
...
+else
+FindSrcDirsForLib = \
+ $(call uniq, $(wildcard \
+ $(JDK_TOPDIR)/src/$(strip $1)/$*(OPENJDK_TARGET_OS)*/native/lib$(strip
$2) \
+ $(JDK_TOPDIR)/src/$(strip
$1)/$(OPENJDK_TARGET_OS_TYPE)/native/lib$(strip $2) \
+ $(JDK_TOPDIR)/src/$(strip $1)/share/native/lib$(strip $2)))
+endif
Will fix the indents there.
jdk/make/lib/SoundLibraries.gmk: indentation (last if block, the rest
looks fine)
Fixed.
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).
Will fix. We just recently stopped using xcodebuild to create a proper
framework.
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.
Not just a library.
Our deliverable artifact is a framework on ios.
The include file and static library are packaged so they can drop
into an Xcode project and all the proper connections are made.
This is the native component that will launch the VM in the current process.
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?
Even though we compile a Java 5 byte code jar file, it will be processed
via dex before it is actually linked into an android executable.
This is the native side that will launch a Java VM within the current
process.
We called in JavaLauncher because it was a replacement for libjli.
I'll investigate the unused OCLDVK_OUTPUTDIR and the WARNINGS_AS_ERRORS.
There is probably some shared linux code that does not compile cleanly
with the android toolchain.
---
This was what I could find right now. I have not looked at hotspot
build changes, nor any source code changes.
Thanks I appreciate all these comments.
My hope is that we will have much smaller reviewable changesets
going forward.
/Magnus
