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.
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
