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

Tue Feb 12 17:10:23 UTC 2019

Hello,

On 2019-02-12 09:05, Severin Gehwolf wrote:
> 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?

Yes, that is what wildcard does, it filters out any non existing dirs.

No need for you to verify anything but that it works as far as I am 
concerned. I'm happy with the below.

/Erik

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