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