Hi Alan, Thanks for the review!
On Fri, 2019-02-08 at 10:08 +0000, Alan Bateman wrote: > On 07/02/2019 17: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 > > > I agree with Mandy that the CLI options need examination. The proposed > `--strip-native-debug-symbols` seems reasonable but it will be > inconsistent with the existing `--strip-debug` option. Mandy - what you > would think about renaming the existing option to something that makes > it clear that it strips the debug attribute from class files? (we would > of course need to do something to keep the existing option working). > > The need to specify the argument as "defaults" or "options" is a bit > annoying. It might be time to replace hasArguments in the plugin > interface to allow for optional arguments. The plugin interface is not > exported/documented/supported so we have flexibility to change it. I've filed this bug for optional arguments: https://bugs.openjdk.java.net/browse/JDK-8218761 It's a separate issue, after all. > If we > do this then it should mean the usages reduce down to: > > --strip-native-debug-symbols > --strip-native-debug-symbols objcopy=<path-to-objcopy>:... > > I see the plugin has moved to src/jdk.jlink/unix in this iteration. It > might be better to start out in src/jdk.jlink/linux - we can always move > to the unix tree in the event that there is interest/support on other > platforms. > > Mandy points out the issue with wrapping in the usage output. I agree > that the`jlink --list-plugins` needs to be readable/tidy esp. in this > case as there are many sub-options to read about. I'll tackle those once there is some agreement as to what the option should be called and which arguments it should have for the initial implementation. I'll reply with a proposal in the other thread. > The implementation will need a bit of a tidy up too. The readFileBytes > method can be replaced with Files.readAllBytes. The BAIS usage can be > replaced with Files.write. Ignoring exceptions thrown in stripBinary > will also need consideration. Should be fixed in the latest webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8214796/05/webrev/ FWIW, I've refrained from using Files.readAllBytes in the initial webrev because of this note in the javadoc: """ Note that this method is intended for simple cases where it is convenient to read all bytes into a byte array. It is not intended for reading in large files. """ Cheers, Severin
