Re: Help with build changes for: 8214796: Create a jlink plugin for stripping debug info symbols from native libraries

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

Re: Help with build changes for: 8214796: Create a jlink plugin for stripping debug info symbols from native libraries

Alan Bateman
On 26/01/2019 00:06, Mandy Chung wrote:
> Hi Severin,
>
> Another alternative would be to support per-jlink-plugin resource
> bundle to avoid merging .properties files at build time.  The
> plugin-specific messages should only be used by the plugin itself
> and it would be cleaner for each plugin to manage its resource bundle.
That seems a cleaner idea than the suggestion we had on jigsaw-dev to
merge the properties. If Severin does this for this plugin then I assume
the resources for the other plugins could be moved to their own resource
files in their own time.

I skimmed the current patch and I see the usability has improved since
the original proposal. It would be nice to get to
`--strip-native-debug-symbols` without needing the "=defaults" suffix. 
There are details around the sub-options and naming that I assume will
need detailed review at some point too.

The current patch doesn't include any tests but I assume they will come
in time.

There's probably a cleanup in StripNativeDebugSymbolsPlugin.java needed
too to make it easier to maintain and review going forward but that is
not important now, I think the main thing is to get past the issue of
being the first plugin that is platform specific.

-Alan.
Reply | Threaded
Open this post in threaded view
|

Re: Help with build changes for: 8214796: Create a jlink plugin for stripping debug info symbols from native libraries

Severin Gehwolf
Hi Alan, Mandy,

On Sun, 2019-01-27 at 08:14 +0000, Alan Bateman wrote:

> On 26/01/2019 00:06, Mandy Chung wrote:
> > Hi Severin,
> >
> > Another alternative would be to support per-jlink-plugin resource
> > bundle to avoid merging .properties files at build time.  The
> > plugin-specific messages should only be used by the plugin itself
> > and it would be cleaner for each plugin to manage its resource bundle.
>
> That seems a cleaner idea than the suggestion we had on jigsaw-dev to
> merge the properties. If Severin does this for this plugin then I assume
> the resources for the other plugins could be moved to their own resource
> files in their own time.

Alright. I'll try this then.

> I skimmed the current patch and I see the usability has improved since
> the original proposal. It would be nice to get to
> `--strip-native-debug-symbols` without needing the "=defaults" suffix.  
> There are details around the sub-options and naming that I assume will
> need detailed review at some point too.

This sounds like a different patch to me, I'll investigate.

> The current patch doesn't include any tests but I assume they will come
> in time.

Sure.

Thanks,
Severin

> There's probably a cleanup in StripNativeDebugSymbolsPlugin.java needed
> too to make it easier to maintain and review going forward but that is
> not important now, I think the main thing is to get past the issue of
> being the first plugin that is platform specific.
>
> -Alan.

Reply | Threaded
Open this post in threaded view
|

Re: Help with build changes for: 8214796: Create a jlink plugin for stripping debug info symbols from native libraries

Alan Bateman
On 28/01/2019 09:26, Severin Gehwolf wrote:
> :
>> I skimmed the current patch and I see the usability has improved since
>> the original proposal. It would be nice to get to
>> `--strip-native-debug-symbols` without needing the "=defaults" suffix.
>> There are details around the sub-options and naming that I assume will
>> need detailed review at some point too.
> This sounds like a different patch to me, I'll investigate.
>
If the common case is to not require any complicated options then
`--strip-native-debug-symbols` would be nice and would be consistent
with the existing `--strip-XXXX` options. The cross targeting case, or
cases where debuginfo options are need, would of course mean a more
complicated command.

-Alan
Reply | Threaded
Open this post in threaded view
|

Re: Help with build changes for: 8214796: Create a jlink plugin for stripping debug info symbols from native libraries

Severin Gehwolf
On Mon, 2019-01-28 at 14:40 +0000, Alan Bateman wrote:

> On 28/01/2019 09:26, Severin Gehwolf wrote:
> > :
> > > I skimmed the current patch and I see the usability has improved since
> > > the original proposal. It would be nice to get to
> > > `--strip-native-debug-symbols` without needing the "=defaults" suffix.
> > > There are details around the sub-options and naming that I assume will
> > > need detailed review at some point too.
> >
> > This sounds like a different patch to me, I'll investigate.
> >
>
> If the common case is to not require any complicated options then
> `--strip-native-debug-symbols` would be nice and would be consistent
> with the existing `--strip-XXXX` options. The cross targeting case, or
> cases where debuginfo options are need, would of course mean a more
> complicated command.

Yes, but I was referring to the current state of the jlink plugin
system. AFAIK, there is two --strip-XXX plugins:
StripNativeCommandsPlugin and StripDebugPlugin. Both of these have
hasArguments() == false. This new one has hasArguments() == true. Then
the plugin system wants at least 1 argument.

What has to be implemented first is a way for a plugin to allow both:
no-args *and* 1+ args. Currently it's 1+ args XOR 0 args. This
functionality doesn't seem related to this particular patch but would
be generic.

Thanks,
Severin

Reply | Threaded
Open this post in threaded view
|

Re: Help with build changes for: 8214796: Create a jlink plugin for stripping debug info symbols from native libraries

Mandy Chung
In reply to this post by Severin Gehwolf


On 1/28/19 1:26 AM, Severin Gehwolf wrote:

> Hi Alan, Mandy,
>
> On Sun, 2019-01-27 at 08:14 +0000, Alan Bateman wrote:
>> On 26/01/2019 00:06, Mandy Chung wrote:
>>> Hi Severin,
>>>
>>> Another alternative would be to support per-jlink-plugin resource
>>> bundle to avoid merging .properties files at build time.  The
>>> plugin-specific messages should only be used by the plugin itself
>>> and it would be cleaner for each plugin to manage its resource bundle.
>> That seems a cleaner idea than the suggestion we had on jigsaw-dev to
>> merge the properties. If Severin does this for this plugin then I assume
>> the resources for the other plugins could be moved to their own resource
>> files in their own time.
> Alright. I'll try this then.

One simplest way is to have StripNativeDebugSymbolsPlugin loads its own
ResourceBundle.

I would add a new PluginsResourceBundle::getMessage method taking a
ResourceBundle object
that StripNativeDebugSymbolsPlugin can call.

This is a patch for PluginsResourceBundle that you can use.

diff --git
a/src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/PluginsResourceBundle.java
b/src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/PluginsResourceBundle.java
---
a/src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/PluginsResourceBundle.java
+++
b/src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/PluginsResourceBundle.java
@@ -62,7 +62,11 @@
      }

      public static String getMessage(String key, Object... args) throws
MissingResourceException {
-        String val = pluginsBundle.getString(key);
+        return getMessage(pluginsBundle, key, args);
+    }
+
+    public static String getMessage(ResourceBundle rb, String key,
Object... args) throws MissingResourceException {
+        String val = rb.getString(key);
          return MessageFormat.format(val, args);
      }
  }

Mandy
Reply | Threaded
Open this post in threaded view
|

RFR: Move jlink plugin resources into their own files (was: Re: Help with build changes for: 8214796: Create a jlink plugin for stripping debug info symbols from native libraries)

Severin Gehwolf
Hi Mandy, Alan,

On Mon, 2019-01-28 at 13:02 -0800, Mandy Chung wrote:

>
> On 1/28/19 1:26 AM, Severin Gehwolf wrote:
> > Hi Alan, Mandy,
> >
> > On Sun, 2019-01-27 at 08:14 +0000, Alan Bateman wrote:
> > > On 26/01/2019 00:06, Mandy Chung wrote:
> > > > Hi Severin,
> > > >
> > > > Another alternative would be to support per-jlink-plugin resource
> > > > bundle to avoid merging .properties files at build time.  The
> > > > plugin-specific messages should only be used by the plugin itself
> > > > and it would be cleaner for each plugin to manage its resource bundle.
> > >
> > > That seems a cleaner idea than the suggestion we had on jigsaw-dev to
> > > merge the properties. If Severin does this for this plugin then I assume
> > > the resources for the other plugins could be moved to their own resource
> > > files in their own time.
> >
> > Alright. I'll try this then.
>  
> One simplest way is to have StripNativeDebugSymbolsPlugin loads its own ResourceBundle.
>
> I would add a new PluginsResourceBundle::getMessage method taking a ResourceBundle object
> that StripNativeDebugSymbolsPlugin can call.
>
> This is a patch for PluginsResourceBundle that you can use.
>
> diff --git a/src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/PluginsResourceBundle.java b/src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/PluginsResourceBundle.java
> --- a/src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/PluginsResourceBundle.java
> +++ b/src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/PluginsResourceBundle.java
> @@ -62,7 +62,11 @@
>      }
>  
>      public static String getMessage(String key, Object... args) throws MissingResourceException {
> -        String val = pluginsBundle.getString(key);
> +        return getMessage(pluginsBundle, key, args);
> +    }
> +
> +    public static String getMessage(ResourceBundle rb, String key, Object... args) throws MissingResourceException {
> +        String val = rb.getString(key);
>          return MessageFormat.format(val, args);
>      }
>  }

Right, thanks! In fact, I've come up with something similar. Here is a
patch which moves resource files for jlink plugins into their own
properties files:

http://cr.openjdk.java.net/~sgehwolf/webrevs/jlink-plugins-resources/01/webrev/

It's not super clean as there is still this weird plugin.opt.<name>
artifact which has to do with TaskHelper and it's use of PluginOption,
but I wanted to keep the patch as minimal as possible.

Following this model, having platform specific plugins should be
easier. Would this be something acceptable upstream? If so, I'll file
an enhancement bug and an official RFR.

I've tested this with tools/jlink/ and jdk/modules tests.

Then the patch for the native debug strip plugin would become this:
http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8214796-wip/02/webrev/index.html

Is this going into the right direction?

Thanks,
Severin

Reply | Threaded
Open this post in threaded view
|

Re: RFR: Move jlink plugin resources into their own files

Mandy Chung


On 1/29/19 2:44 AM, Severin Gehwolf wrote:

> Right, thanks! In fact, I've come up with something similar. Here is a
> patch which moves resource files for jlink plugins into their own
> properties files:
>
> http://cr.openjdk.java.net/~sgehwolf/webrevs/jlink-plugins-resources/01/webrev/
>
> It's not super clean as there is still this weird plugin.opt.<name>
> artifact which has to do with TaskHelper and it's use of PluginOption,
> but I wanted to keep the patch as minimal as possible.
>
> Following this model, having platform specific plugins should be
> easier. Would this be something acceptable upstream? If so, I'll file
> an enhancement bug and an official RFR.


Thanks for doing this.  Yes this should be a separate RFE.
I think each plugin should have its own ResourceBundle instance
as jlink should use the Plugin API to get the description, arguments
etc for the output of --list-plugins.  It does not need to maintain
plugin to ResourceBundle map.   It may involve more cleanup that
I will have to look at the current implementation to give any
suggestion.

I think your patch does not necessarily depend on this refactoring
and it makes sense to do this RFE separately and get the strip native
debug symbol plugin in first.


> I've tested this with tools/jlink/ and jdk/modules tests.
>
> Then the patch for the native debug strip plugin would become this:
> http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8214796-wip/02/webrev/index.html
>
> Is this going into the right direction?

Looks reasonable.  I will review it and send feedback.

W.r.t. plugin resources, I suggest to have this plugin to maintain
its own ResourceBundle and format its own message so that this can
proceed independently with the existing plugin resource refactoring
work.  PluginsResourceBundle methods are convenient methods which
this new plugin does not have to use it.

Mandy