RFR: 8214796: Create a jlink plugin for stripping debug info symbols from native libraries

[Severin Gehwolf]

Hi Erik,

On Tue, 2019-02-12 at 08:44 -0800, Erik Joelsson wrote:
> On 2019-02-12 01:35, Severin Gehwolf wrote:
> > Hi Erik,
> > 
> > On Mon, 2019-02-11 at 11:12 -0800, Erik Joelsson wrote:
> > > Hello Severin,
> > > 
> > > I think you also need a $(wildcard ) around it, but I may be wrong. Did
> > > you try this version?
> > Yes, this version works for me without $(wildcard). Why is it needed?
> 
> I had to go back and check again to verify, but now I'm sure. The list 
> of directories returned by FindModuleSrcDirs is all src dirs for the 
> module. Not all of them are going to contain the specific directory 
> jdk/tools/jlink/resources. This means SetupCompileProperties will get 
> called with a few non existing directories. While this will work fine, 
> the find implementation on some platforms will complain (Macos in 
> particular), so to avoid introducing confusing warning messages in the 
> build, it's better to filter down the list of directories to those that 
> actually exist.

OK, thanks for the explanation. I suppose $(wildcard ...) does that,
then? I've added it back locally but I have no way of testing whether
this makes any difference, except jdk/submit perhaps?

diff --git a/make/gensrc/Gensrc-jdk.jlink.gmk b/make/gensrc/Gensrc-jdk.jlink.gmk
--- a/make/gensrc/Gensrc-jdk.jlink.gmk
+++ b/make/gensrc/Gensrc-jdk.jlink.gmk
@@ -29,8 +29,9 @@
 
 ################################################################################
 
-JLINK_RESOURCES_DIRS := $(addsuffix /jdk/tools/jlink/resources, \
-    $(call FindModuleSrcDirs, jdk.jlink))
+# Use wildcard so as to avoid getting non-existing directories back
+JLINK_RESOURCES_DIRS := $(wildcard $(addsuffix /jdk/tools/jlink/resources, \
+    $(call FindModuleSrcDirs, jdk.jlink)))
 
 $(eval $(call SetupCompileProperties, JLINK_PROPERTIES, \
     SRC_DIRS := $(JLINK_RESOURCES_DIRS), \

Thanks,
Severin

> > > Also, please do not indent so much. We have style guidelines [1], which
> > > recommend 4 spaces for line break indentation.
> > OK, sorry. Fixed locally.
> 
> Thanks!
> 
> /Erik
> 
> > Thanks,
> > Severin
> > 
> > > /Erik
> > > 
> > > [1] http://openjdk.java.net/groups/build/doc/code-conventions.html
> > > 
> > > On 2019-02-11 10:03, Severin Gehwolf wrote:
> > > > Hi Erik,
> > > > 
> > > > On Thu, 2019-02-07 at 17:01 -0800, Erik Joelsson wrote:
> > > > > On 2019-02-07 11:09, Severin Gehwolf wrote:
> > > > > > Hi Erik,
> > > > > > 
> > > > > > On Thu, 2019-02-07 at 09:39 -0800, Erik Joelsson wrote:
> > > > > > > Hello Severin,
> > > > > > > 
> > > > > > > There is a macro for automatically finding all source dirs for a module.
> > > > > > > So in Gensrc-jdk.jlink.gmk, I think it would be better expressed using
> > > > > > > that macro, like this:
> > > > > > > 
> > > > > > > JLINK_RESOURCE_DIRS := $(wildcard $(addsuffix
> > > > > > > /jdk/tools/jlink/resources, $(call FindModuleSrcSdirs, jdk.jlink)))
> > > > > > > 
> > > > > > > The above could/should even be inlined.
> > > > > > I've considered this. It seems, though, that FindModuleSrcDirs comes
> > > > > > from make/common/Modules.gmk which isn't included in
> > > > > > make/gensrc/Gensrc-jdk.jlink.gmk. Given that it has already caused
> > > > > > problems with multiple includes of Modules.gmk (JDK-8213736) I was
> > > > > > reluctant to include it here too. Without the new include the above
> > > > > > won't work.
> > > > > > 
> > > > > > The approach I've taken here seems to be the lesser evil. Thoughts?
> > > > > I appreciate your concern, but JDK-8213736 was a problem in Main.gmk,
> > > > > which is part of where Modules.gmk gets bootstrapped. In any normal
> > > > > makefile (as in where stuff is just being built), the bootstrapping is
> > > > > done and including Modules.gmk is completely fine. Any
> > > > > <phase>-<module>.gmk file certainly qualifies here.
> > > > OK. Updated:
> > > > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8214796/05/
> > > > 
> > > > Thanks,
> > > > Severin
> > > > 
> > > > > /Erik
> > > > > 
> > > > > > Thanks,
> > > > > > Severin
> > > > > > 
> > > > > > > Otherwise build changes look ok.
> > > > > > > 
> > > > > > > /Erik
> > > > > > > 
> > > > > > > On 2019-02-07 09:09, Severin Gehwolf wrote:
> > > > > > > > Hi,
> > > > > > > > 
> > > > > > > > Could I please get reviews for this enhancement? It adds a
> > > > > > > > debug
> > > > > > > > symbols stripping plug-in to jlink for Linux and Unix
> > > > > > > > platforms. It's
> > > > > > > > the first platform specific jlink plugin and the approach taken
> > > > > > > > for
> > > > > > > > keeping it contained is to use a plugin specific
> > > > > > > > ResourceBundle.
> > > > > > > > Discussion for this happened in [1].
> > > > > > > > 
> > > > > > > > The test uses a native library which should never get debug
> > > > > > > > symbols
> > > > > > > > stripped during the test library build. As such, tiny
> > > > > > > > modifications
> > > > > > > > were needed to make/common/TestFilesCompilation.gmk. Hence,
> > > > > > > > build-dev
> > > > > > > > being on the list for this RFR. The test currently only runs on
> > > > > > > > Linux
> > > > > > > > and requires objcopy to be available. Otherwise the test is
> > > > > > > > being
> > > > > > > > skipped.
> > > > > > > > 
> > > > > > > > Example usage of this plugin is described in the bug.
> > > > > > > > 
> > > > > > > > webrev:
> > > > > > > > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8214796/04/webrev/
> > > > > > > > Bug: https://bugs.openjdk.java.net/browse/JDK-8214796
> > > > > > > > 
> > > > > > > > Testing: test/jdk/tools/jlink test/jdk/jdk/modules tests on
> > > > > > > > Linux
> > > > > > > > x86_64 (with good and broken objcopy) and the newly added test.
> > > > > > > > It's
> > > > > > > > currently running through jdk/submit too.
> > > > > > > > 
> > > > > > > > Thoughts?
> > > > > > > > 
> > > > > > > > Thanks,
> > > > > > > > Severin
> > > > > > > > 
> > > > > > > > [1]
> > > > > > > > http://mail.openjdk.java.net/pipermail/jigsaw-dev/2019-January/014109.html
> > > > > > > >