RFR: 8218913: Rename --strip-debug jlink plugin

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

RFR: 8218913: Rename --strip-debug jlink plugin

Severin Gehwolf
Hi,

As discussed in the RFR thread for JDK-8214796 it was suggested to
rename the --strip-debug plugin so that --strip-debug could actually
perform java debug symbols stripping *and* native debug symbols
stripping at once. That's what this patch does. It renames --strip-
debug to --strip-java-debug-symbols and --strip-debug just calls --
strip-java-debug-symbols behind the scenes.

Note that --strip-debug-attributes as name would work for me too, but
given that we are about to introduce --strip-native-debug-symbols,
having the java strippping plugin with a similar name made sense to me.

bug: https://bugs.openjdk.java.net/browse/JDK-8218913
webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8218913/01/webrev/

Testing: jlink/jimage tests and jdk/submit (currently running).

Thoughts?

Thanks,
Severin

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8218913: Rename --strip-debug jlink plugin

Mandy Chung
Hi Severin,

Thanks for doing this.  I review the webrev.

You add a new DefaultStripDebugPlugin that will do the work.
I have been assuming that this can be implemented as a jlink option
that get translated into --strip-java-debug-symbols during
the command line argument processing.

I'm okay with --strip-java-debug-symbols.  The help message from
javac -g option:
   -g                           Generate all debugging info

The other choice is --strip-java-debug-info and
--strip-native-debug-info.  We can settle the naming in the other thread
to get others
opinion.

Mandy

On 2/13/19 3:16 AM, Severin Gehwolf wrote:

> Hi,
>
> As discussed in the RFR thread for JDK-8214796 it was suggested to
> rename the --strip-debug plugin so that --strip-debug could actually
> perform java debug symbols stripping *and* native debug symbols
> stripping at once. That's what this patch does. It renames --strip-
> debug to --strip-java-debug-symbols and --strip-debug just calls --
> strip-java-debug-symbols behind the scenes.
>
> Note that --strip-debug-attributes as name would work for me too, but
> given that we are about to introduce --strip-native-debug-symbols,
> having the java strippping plugin with a similar name made sense to me.
>
> bug: https://bugs.openjdk.java.net/browse/JDK-8218913
> webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8218913/01/webrev/
>
> Testing: jlink/jimage tests and jdk/submit (currently running).
>
> Thoughts?
>
> Thanks,
> Severin
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8218913: Rename --strip-debug jlink plugin

Severin Gehwolf
Hi Mandy,

On Wed, 2019-02-13 at 15:04 -0800, Mandy Chung wrote:
> Hi Severin,
>
> Thanks for doing this.  I review the webrev.

Thanks for the review!

> You add a new DefaultStripDebugPlugin that will do the work.
> I have been assuming that this can be implemented as a jlink option
> that get translated into --strip-java-debug-symbols during
> the command line argument processing.

The reason I've opted for this approach was the intention to hook --
strip-debug up with not only --strip-java-debug-symbols, but also --
strip-native-debug-symbols as well once that gets in. The analogy to
the compress plugin applies here. Using this approach will make that
additional hooking up easier. Though, thinking about this some more --
strip-native-debug-info will be linux-only, so I'll have to ponder a
bit about how to make use of that platform-specific option without
breaking platforms which don't have it.

> I'm okay with --strip-java-debug-symbols.  The help message from
> javac -g option:
>    -g                           Generate all debugging info
>
> The other choice is --strip-java-debug-info and
> --strip-native-debug-info.  We can settle the naming in the other thread
> to get others
> opinion.

As indicated in the other thread, I'd be happy to call it --strip-java-
debug-info and --strip-native-debug-info. I'll wait for the other
thread to settle before I'll update the webrev.

Thanks,
Severin

> Mandy
>
> On 2/13/19 3:16 AM, Severin Gehwolf wrote:
> > Hi,
> >
> > As discussed in the RFR thread for JDK-8214796 it was suggested to
> > rename the --strip-debug plugin so that --strip-debug could actually
> > perform java debug symbols stripping *and* native debug symbols
> > stripping at once. That's what this patch does. It renames --strip-
> > debug to --strip-java-debug-symbols and --strip-debug just calls --
> > strip-java-debug-symbols behind the scenes.
> >
> > Note that --strip-debug-attributes as name would work for me too, but
> > given that we are about to introduce --strip-native-debug-symbols,
> > having the java strippping plugin with a similar name made sense to me.
> >
> > bug: https://bugs.openjdk.java.net/browse/JDK-8218913
> > webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8218913/01/webrev/
> >
> > Testing: jlink/jimage tests and jdk/submit (currently running).
> >
> > Thoughts?
> >
> > Thanks,
> > Severin
> >

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8218913: Rename --strip-debug jlink plugin

Alan Bateman
In reply to this post by Severin Gehwolf
On 13/02/2019 11:16, Severin Gehwolf wrote:

> Hi,
>
> As discussed in the RFR thread for JDK-8214796 it was suggested to
> rename the --strip-debug plugin so that --strip-debug could actually
> perform java debug symbols stripping *and* native debug symbols
> stripping at once. That's what this patch does. It renames --strip-
> debug to --strip-java-debug-symbols and --strip-debug just calls --
> strip-java-debug-symbols behind the scenes.
>
> Note that --strip-debug-attributes as name would work for me too, but
> given that we are about to introduce --strip-native-debug-symbols,
> having the java strippping plugin with a similar name made sense to me.
>
> bug: https://bugs.openjdk.java.net/browse/JDK-8218913
> webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8218913/01/webrev/
>
I skimmed through the webrev and the approach looks good.

I don't have a strong opinion on the naming but the previous suggestion
to use --strip-java-debug-attributes seems slightly better, if only it
gives a hint that it strips the LVT and LLVT class file attributes. If
the "strip the world" option is --strip-debug-info then it could imply
both --strip-java-debug-attributes and --strip-native-debug-symbols
without needing to unify the terminology.

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

Re: RFR: 8218913: Rename --strip-debug jlink plugin

Severin Gehwolf
Hi Alan,

On Thu, 2019-02-14 at 15:26 +0000, Alan Bateman wrote:

> On 13/02/2019 11:16, Severin Gehwolf wrote:
> > Hi,
> >
> > As discussed in the RFR thread for JDK-8214796 it was suggested to
> > rename the --strip-debug plugin so that --strip-debug could actually
> > perform java debug symbols stripping *and* native debug symbols
> > stripping at once. That's what this patch does. It renames --strip-
> > debug to --strip-java-debug-symbols and --strip-debug just calls --
> > strip-java-debug-symbols behind the scenes.
> >
> > Note that --strip-debug-attributes as name would work for me too, but
> > given that we are about to introduce --strip-native-debug-symbols,
> > having the java strippping plugin with a similar name made sense to me.
> >
> > bug: https://bugs.openjdk.java.net/browse/JDK-8218913
> > webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8218913/01/webrev/
> >
> I skimmed through the webrev and the approach looks good.

Thanks for the review!

> I don't have a strong opinion on the naming but the previous suggestion
> to use --strip-java-debug-attributes seems slightly better, if only it
> gives a hint that it strips the LVT and LLVT class file attributes. If
> the "strip the world" option is --strip-debug-info then it could imply
> both --strip-java-debug-attributes and --strip-native-debug-symbols
> without needing to unify the terminology.

A bit of clarification would be needed here I think:

1)
The current option is called '--strip-debug', not '--strip-debug-info'.
Did you mean to say to rename --strip-debug to --strip-debug-info too?
Perhaps make '--strip-debug' and alias for '--strip-debug-info'?

2)
--strip-debug-attributes as a replacement for what --strip-debug does
today was discussed. The above patch uses --strip-java-debug-symbols.
Now you say --strip-java-debug-attributes. Is that the name I should be
using?

Currently, my understanding is to go with:

--strip-debug => --strip-java-debug-attributes and let --strip-debug
just call the --strip-java-debug-attributes plugin. --strip-debug-info
would not exist (as it doesn't exist now).

Thanks,
Severin

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8218913: Rename --strip-debug jlink plugin

Mandy Chung
In reply to this post by Severin Gehwolf


On 2/14/19 1:04 AM, Severin Gehwolf wrote:

> Hi Mandy,
>
>> You add a new DefaultStripDebugPlugin that will do the work.
>> I have been assuming that this can be implemented as a jlink option
>> that get translated into --strip-java-debug-symbols during
>> the command line argument processing.
>
> The reason I've opted for this approach was the intention to hook --
> strip-debug up with not only --strip-java-debug-symbols, but also --
> strip-native-debug-symbols as well once that gets in. The analogy to
> the compress plugin applies here. Using this approach will make that
> additional hooking up easier. Though, thinking about this some more --
> strip-native-debug-info will be linux-only, so I'll have to ponder a
> bit about how to make use of that platform-specific option without
> breaking platforms which don't have it.

Good point.

That's the merit of this approach which makes it easier to hook
up a platform-specific plugin that may or may not exist.

Mandy
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8218913: Rename --strip-debug jlink plugin

Mandy Chung
In reply to this post by Severin Gehwolf
On 2/14/19 8:38 AM, Severin Gehwolf wrote:

> A bit of clarification would be needed here I think:
>
> 1)
> The current option is called '--strip-debug', not '--strip-debug-info'.
> Did you mean to say to rename --strip-debug to --strip-debug-info too?
> Perhaps make '--strip-debug' and alias for '--strip-debug-info'?
>
> 2)
> --strip-debug-attributes as a replacement for what --strip-debug does
> today was discussed. The above patch uses --strip-java-debug-symbols.
> Now you say --strip-java-debug-attributes. Is that the name I should be
> using?
>
> Currently, my understanding is to go with:
>
> --strip-debug => --strip-java-debug-attributes and let --strip-debug
> just call the --strip-java-debug-attributes plugin. --strip-debug-info
> would not exist (as it doesn't exist now).

--strip-debug is the documented jlink option and I don't think
we need a new --strip-debug-info option.

--strip-java-debug-attributes and --strip-native-debug-symbols
are very clear.  I'm happy to go with them.

thanks
Mandy

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8218913: Rename --strip-debug jlink plugin

Alan Bateman
In reply to this post by Severin Gehwolf
On 14/02/2019 16:38, Severin Gehwolf wrote:
> :
> A bit of clarification would be needed here I think:
>
> 1)
> The current option is called '--strip-debug', not '--strip-debug-info'.
> Did you mean to say to rename --strip-debug to --strip-debug-info too?
> Perhaps make '--strip-debug' and alias for '--strip-debug-info'?
Sorry, there a typo in my mail, I meant `--strip-debug`.
>
> Currently, my understanding is to go with:
>
> --strip-debug => --strip-java-debug-attributes and let --strip-debug
> just call the --strip-java-debug-attributes plugin. --strip-debug-info
> would not exist (as it doesn't exist now).
>
That seems good to me. I see Mandy is on board with this plan and naming
too.

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

Re: RFR: 8218913: Rename --strip-debug jlink plugin

Severin Gehwolf
In reply to this post by Mandy Chung
Hi Mandy,

On Wed, 2019-02-13 at 15:04 -0800, Mandy Chung wrote:
> Hi Severin,
>
> Thanks for doing this.  I review the webrev.

Here is the latest webrev:
http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8218913/02/webrev/

It uses --strip-java-debug-attributes as discussed in [1]. The --list-
plugins output for --strip-debug and --strip-java-debug-attributes
reads as follows:

Plugin Name: strip-debug
Option: --strip-debug
Description: Strip debug information from the output image

Plugin Name: strip-java-debug-attributes
Option: --strip-java-debug-attributes
Description: Strip Java debug attributes from classes in the output image

Testing: test/jdk/tools/jlink and jdk/submit.

Thoughts?

Thanks,
Severin

[1] http://mail.openjdk.java.net/pipermail/jigsaw-dev/2019-February/014152.html

> You add a new DefaultStripDebugPlugin that will do the work.
> I have been assuming that this can be implemented as a jlink option
> that get translated into --strip-java-debug-symbols during
> the command line argument processing.
>
> I'm okay with --strip-java-debug-symbols.  The help message from
> javac -g option:
>    -g                           Generate all debugging info
>
> The other choice is --strip-java-debug-info and
> --strip-native-debug-info.  We can settle the naming in the other
> thread
> to get others
> opinion.
>
> Mandy
>
> On 2/13/19 3:16 AM, Severin Gehwolf wrote:
> > Hi,
> >
> > As discussed in the RFR thread for JDK-8214796 it was suggested to
> > rename the --strip-debug plugin so that --strip-debug could
> > actually
> > perform java debug symbols stripping *and* native debug symbols
> > stripping at once. That's what this patch does. It renames --strip-
> > debug to --strip-java-debug-symbols and --strip-debug just calls --
> > strip-java-debug-symbols behind the scenes.
> >
> > Note that --strip-debug-attributes as name would work for me too,
> > but
> > given that we are about to introduce --strip-native-debug-symbols,
> > having the java strippping plugin with a similar name made sense to
> > me.
> >
> > bug: https://bugs.openjdk.java.net/browse/JDK-8218913
> > webrev:
> > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8218913/01/webrev/
> >
> > Testing: jlink/jimage tests and jdk/submit (currently running).
> >
> > Thoughts?
> >
> > Thanks,
> > Severin
> >

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8218913: Rename --strip-debug jlink plugin

Alan Bateman
On 15/02/2019 12:15, Severin Gehwolf wrote:

> :
> Here is the latest webrev:
> http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8218913/02/webrev/
>
> It uses --strip-java-debug-attributes as discussed in [1]. The --list-
> plugins output for --strip-debug and --strip-java-debug-attributes
> reads as follows:
>
> Plugin Name: strip-debug
> Option: --strip-debug
> Description: Strip debug information from the output image
>
> Plugin Name: strip-java-debug-attributes
> Option: --strip-java-debug-attributes
> Description: Strip Java debug attributes from classes in the output image
>
This looks good to me. One minor point is that try to discourage @author
tags in source files these days.

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

Re: RFR: 8218913: Rename --strip-debug jlink plugin

Severin Gehwolf
Hi Alan,

On Fri, 2019-02-15 at 15:57 +0000, Alan Bateman wrote:

> On 15/02/2019 12:15, Severin Gehwolf wrote:
> > :
> > Here is the latest webrev:
> > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8218913/02/webrev/
> >
> > It uses --strip-java-debug-attributes as discussed in [1]. The --list-
> > plugins output for --strip-debug and --strip-java-debug-attributes
> > reads as follows:
> >
> > Plugin Name: strip-debug
> > Option: --strip-debug
> > Description: Strip debug information from the output image
> >
> > Plugin Name: strip-java-debug-attributes
> > Option: --strip-java-debug-attributes
> > Description: Strip Java debug attributes from classes in the output image
> >
> This looks good to me. One minor point is that try to discourage @author
> tags in source files these days.

Thanks for the review! I've remove the @author tag:
http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8218913/03/webrev/

Thanks,
Severin

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8218913: Rename --strip-debug jlink plugin

Alan Bateman
On 15/02/2019 16:12, Severin Gehwolf wrote:
> :
> Thanks for the review! I've remove the @author tag:
> http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8218913/03/webrev/
>
Looks good. Are you planning to do submit a CSR for this? (CLI options
are a supported/documented interface).

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

Re: RFR: 8218913: Rename --strip-debug jlink plugin

Mandy Chung
In reply to this post by Severin Gehwolf


On 2/15/19 8:12 AM, Severin Gehwolf wrote:
> Thanks for the review! I've remove the @author tag:
> http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8218913/03/webrev/

Looks good.  As Alan said, this needs a CSR to document the new plugin
command-line option.

thanks
Mandy
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8218913: Rename --strip-debug jlink plugin

Severin Gehwolf
On Fri, 2019-02-15 at 23:27 -0800, Mandy Chung wrote:
> On 2/15/19 8:12 AM, Severin Gehwolf wrote:
> > Thanks for the review! I've remove the @author tag:
> > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8218913/03/webrev/
>
> Looks good.  As Alan said, this needs a CSR to document the new plugin
> command-line option.

Thanks. CSR has been filed here:
https://bugs.openjdk.java.net/browse/JDK-8219207

Cheers,
Severin

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8218913: Rename --strip-debug jlink plugin

Alan Bateman
On 18/02/2019 09:50, Severin Gehwolf wrote:
> :
> Thanks. CSR has been filed here:
> https://bugs.openjdk.java.net/browse/JDK-8219207
>
The interface change is to add --strip-java-debug-attributes so it might
be better to change the synopsis and the summary to make this clearer.
Also in the specification section it would be better to include the
usage message for the new option rather than a link to the webrev (as
the webrev could disappear at any time and so not useful to anyone
looking at the CSR at some future time).

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

Re: RFR: 8218913: Rename --strip-debug jlink plugin

Severin Gehwolf
On Mon, 2019-02-18 at 10:10 +0000, Alan Bateman wrote:

> On 18/02/2019 09:50, Severin Gehwolf wrote:
> > :
> > Thanks. CSR has been filed here:
> > https://bugs.openjdk.java.net/browse/JDK-8219207
> >
>  The interface change is to add --strip-java-debug-attributes so it
> might be better to change the synopsis and the summary to make this
> clearer. Also in the specification section it would be better to
> include the usage message for the new option rather than a link to
> the webrev (as the webrev could disappear at any time and so not
> useful to anyone looking at the CSR at some future time).

OK. Thanks, Alan. I've updated it accordingly.

Thanks,
Severin

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8218913: Rename --strip-debug jlink plugin

Mandy Chung
In reply to this post by Severin Gehwolf


On 2/18/19 1:50 AM, Severin Gehwolf wrote:

> On Fri, 2019-02-15 at 23:27 -0800, Mandy Chung wrote:
>> On 2/15/19 8:12 AM, Severin Gehwolf wrote:
>>> Thanks for the review! I've remove the @author tag:
>>> http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8218913/03/webrev/
>>
>> Looks good.  As Alan said, this needs a CSR to document the new plugin
>> command-line option.
>
> Thanks. CSR has been filed here:
> https://bugs.openjdk.java.net/browse/JDK-8219207

Reviewed.

Mandy