Hi Magnus,
On 17/09/2019 9:26 pm, Magnus Ihse Bursie wrote:
On 2019-09-17 01:01, David Holmes wrote:
Hi Christoph,
Sorry for the delay getting back you.
cc'd build-dev to get some clarification on the below ...
On 12/09/2019 7:30 pm, Langer, Christoph wrote:
Hi David,
please review an enhancement which I've identified when working with
Processhelper for JDK-8230850.
I noticed that ProcessHelper is an interface in common code with a
static method that would lookup the actual platform implementation via
reflection. This seems a little cumbersome since we can have a common
dummy for ProcessHelper and override it with the platform specific
implementation, leveraging the build system.
I don't see you leveraging the build system. You have two source files
that compile to the same destination class file. What is ensuring the
platform specific version is compiled after the generic one?
Service-provider patterns use reflection to instantiate the service
implementation. I don't see any problem here that needs solving.
TL;DR:
There are two source files, one in share/classes and one in
linux/classes. The build system overrides the share/classes
implementation with the linux/classes implementation in the linux
build. This is not by coincidence and only one class is contained in
the generated jdk.jcmd module. Then there won't be a need for having
a service interface and a service implementation that is looked up
via reflection (which is not a bad pattern by itself). I agree that
it's not a big problem to be solved but still not "no problem".
Here is some longer elaboration how the build system prefers specific
implementations of classes and filters generic duplicates:
The SetupJavaCompilation function from JavaCompilation.gmk [0] is
used to compile the java sources for JDK modules. In its
documentation, for argument SRC [1], it claims: "one or more
directories to search for sources. The order of the source roots is
significant. The first found file of a certain name has priority". In
its implementation the found files are first ordered [3] and
duplicates filtered out [4].
The potential source files are handed to SetupJavaCompilation in
CompileJavaModules.gmk [5] and were collected by a call to
FindModuleSrcDirs [6]. FindModuleSrcDirs iterates over all potential
source dirs for Java classes in the module [7]. The evaluated subdirs
are (in that order) $(OPENJDK_TARGET_OS)/classes,
$(OPENJDK_TARGET_OS_TYPE)/classes and share/classes, as per [8].
Hope that explains what I'm trying to leverage here.
I'm not 100% certain that what you describe actually ensures what you
want it to ensure. I can't reconcile "the first found file ... has
priority" with the fact found files are sorted and duplicates
eliminated. It is the sorting that concerns me as it suggests
linux/Foo.java might replace shared/Foo.java, but if we're on Windows
then we have a problem! That said there is also this comment:
# Order src files according to the order of the src dirs. Correct
odering is
# needed for correct overriding between different source roots.
I'd need the build team to clarify what "correct overriding" is
actually defined as.
David,
Christoph is correct. linux/Foo.java will override share/Foo.java. I
don't remember how the magic in JavaCompilation.gmk works anymore :-),
but we have relied on this behavior in other places for a long time, so
I'm pretty certain it is still working correctly. Presumably, the $(sort
...) is there to remove (identical) duplicates, which is a side-effect
of sort.
Thanks for confirming. I'd still like to understand exactly what these
overriding rules are though. It's not a mechanism I was aware of.
Thanks,
David
/Magnus
Thanks,
David
-----
I've uploaded an updated webrev which contains some cleanup to the
Test changes: http://cr.openjdk.java.net/~clanger/webrevs/8230857.1/
Thanks
Christoph
[0]
http://hg.openjdk.java.net/jdk/jdk/file/ea93d6a9f720/make/common/JavaCompilation.gmk#l185
[1]
http://hg.openjdk.java.net/jdk/jdk/file/ea93d6a9f720/make/common/JavaCompilation.gmk#l157
[3]
http://hg.openjdk.java.net/jdk/jdk/file/ea93d6a9f720/make/common/JavaCompilation.gmk#l225
[4]
http://hg.openjdk.java.net/jdk/jdk/file/ea93d6a9f720/make/common/JavaCompilation.gmk#l257
[5]
http://hg.openjdk.java.net/jdk/jdk/file/ea93d6a9f720/make/CompileJavaModules.gmk#l603
[6]
http://hg.openjdk.java.net/jdk/jdk/file/ea93d6a9f720/make/CompileJavaModules.gmk#l555
[7]
http://hg.openjdk.java.net/jdk/jdk/file/ea93d6a9f720/make/common/Modules.gmk#l300
[8]
http://hg.openjdk.java.net/jdk/jdk/file/ea93d6a9f720/make/common/Modules.gmk#l243
