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

[sgehwolf at redhat.com]

Hi Alan,

Thank you very much for the review!

New webrev:
http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8214796/10/webrev/

Answers to your comments below.

On Mon, 2019-05-06 at 20:03 +0100, Alan Bateman wrote:
> On 09/04/2019 10:06, Severin Gehwolf wrote:
> > :
> > > Hi Severin,
> > > 
> > > This is my initial set of comments.
> > Thanks for the review! Latest webrev with the updates:
> > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8214796/09/webrev/
> > 
> > 
> I went through 09/webrev and just have a few comments, mostly on 
> StripNativeDebugSymbolsPlugin.
> 
> stripBinary uses ProcessBuilder to launch objdump but it doesn't 
> redirect or read from the process streams. This could make diagnosing 
> problems difficult so I think it should minimally use 
> redirectOutput(INHERIT) and redirectError(INHERIT) so that any 
> output/errors can be seen by the jlink user.

OK. Done.

> At L180 there is a question to the reader on whether arg can be null. It 
> could be good to answer that if possible and avoid needing to think 
> about that.

Answered. It must never be null currently. Once it is, it's a bug.
Hence, IllegalStateException being thrown.

> At L357 there mix of old and new - I assume you can be replaced with 
> inFile.toAbsolutePath().toString();

OK. Fixed.

> Can you explain the mocking in DefaultStripDebugPluginTest? I think I've 
> missed the reason for this test.

So StripNativeDebugSymbolsPlugin is the first platform specific plugin.
Currently Linux only. DefaultStripDebugPlugin (-G or --strip-debug), on
the other hand, is available everywhere. On Linux, --strip-debug does
native *and* java debug attributes stripping. On other platforms it
only strips java debug attributes.

The test verifies both cases irrespective of the plaform it runs on.
I've opted for mocking here since running the real plugins isn't needed
for these simple tests: "two plugins run on platform X", "one plugin
runs on platform Y". Does that answer your question?

FWIW, since DefaultStripDebugPlugin is being run via IntegrationTest
running the real plugins is covered too.

> In passing, we try to avoid the @author tag as it's so difficult to 
> remove them (even if a test is completely re-worked).

Sure. I've removed all @author tags I could find.

Thanks,
Severin