Hi Mandy, Thanks for the review! I'll update the webrev ASAP and will get back to you once ready.
Thanks, Severin On Tue, 2019-02-26 at 13:35 -0800, Mandy Chung wrote: > Hi Severin, > > This is my initial set of comments. > > On 2/26/19 3:01 AM, Severin Gehwolf wrote: > > --list-plugins output for --strip-native-debug-symbols: > > > > Plugin Name: strip-native-debug-symbols > > Option: --strip-native-debug-symbols=<exclude-debuginfo-files|keep- > > debuginfo-files> > > Description: Strip debug symbols from native libraries (if any). > > This plugin requires at least one option: > > objcopy: The path to the objcopy binary. Defaults to objcopy in > > PATH. > > exclude-debuginfo-files: Omit debug info files. Defaults to > > true. > > s/Omit/Exclude/ > > keep-debuginfo-files[=<ext>]: Keep debug info files in > > <file>.<ext>. > > Defaults to <file>.debuginfo > > Examples: --strip-native-debug-symbols objcopy=/usr/bin/objcopy > > This is a little awkward that this defaults to exclude-debuginfo- > files. As described above, I would expect "exclude-debuginfo-files" > or "keep-debuginfo-files" must be specified. It means that the > command would be: > --strip-native-debug-symbols exclude-debuginfo- > files:objcopy=/usr/bin/objcopy > > --strip-native-debug-symbols=exclude-debuginfo-files > > --strip-native-debug-symbols keep-debuginfo- > > files:objcopy=objcopy > > > > What is the expected behavior when jlink --strip-debug --strip- > native-debug-symbols keep-debuginfo-files is specified? Should it > be accepted? This should be specified in the CSR. > > Latest webrev which implements this: > > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8214796/08/webrev/ > > > > DefaultStripDebugPlugin.java > > I still think that conceptually it's more straight-forward to treat > --strip-debug option to imply as if --strip-java-debug-attributes and > --strip-native-debug-symbols option, if available and not specified, > were specified on the command line. These plugins will be invoked > accordingly rather than an explicit invocation of these 2 plugins. > But we don't have such precedence to follow. W.r.t. your current > implementation: > > I think this can be simplfied to keep Optional<Plugin> field and > replace > DefaultNativePluginFactory and NativePluginFactory with a simple > static > method to return Optional.ofNullable(...). > > 47 private static final String OMIT_DEBUGINFO = "exclude- > debuginfo-files"; > > Nit: rename OMIT_ to EXCLUDE_ to match the name > > 77 Map<String, String> stripNativeConfig = Map.<String, > String>of( > > drop the diamond operation. > > StripNativeDebugSymbolsPlugin.java > > 56 public static final String NAME = "strip-native-debug- > symbols"; > > Does this need to be public? Should it be private static field? > > 101 if (resource.type() == > ResourcePoolEntry.Type.NATIVE_LIB || > 102 resource.type() == > ResourcePoolEntry.Type.NATIVE_CMD) { > 103 String moduleName = resource.moduleName(); > 104 if (resource.path().endsWith(SHARED_LIBS_EXT) || > 105 resource.path().startsWith("/" + moduleName > + "/bin")) { > > > Alternatively: > > if ((resource.type() == ResourcePoolEntry.Type.NATIVE_LIB > && resource.path().endsWith(SHARED_LIBS_EXT)) || > resource.type() == ResourcePoolEntry.Type.NATIVE_CMD) > > > If --strip-native-commands is specified, this may want to skip > processing NATIVE_CMD? Alternatively it should ensure -strip-native- > commands is invoked before --strip-native-debug-symbols; otherwise, > debuginfo files might have left behind if keep-debuginfo-files is > specified while the native commands are stripped later. > > doConfigure compares if arg matches one of the expected argument > names. I think it does not report if any invalid argument? > > Looks like it also accepts --strip-native-debug-symbols=exclude- > debuginfo-files=abc:keep-debuginfo-files=cde. This might be a jlink > plugin parsing bug? > > createDebugSymbolsFile, addGnuDebugLink and stripBinary > > - if the command fails, should the plugin fail instead? you currently > silently ignore any error. > > ObjCopyCmdBuilder.build(String objCopy) can be changed to > ObjCopyCmdBuilder.build(String objCopy, String... opts) to build the > entire command line in one go. > > Nit: StrippedDebugInfoBinary can be made immutable. It might be > cleaner to refactor the createDebugSymbolsFile, addGnuDebugLink and > stripBinary methods in a builder class to build a > StrippedDebugInfoBinary for a given ResourcePoolEntry. > > I suggest to place the tests under > test/jdk/tools/jlink/plugins/StripNativeDebugSymbolsPlugin/ which I > think jlink tests are organized with plugin class name rather than > plugin option name. > > Mandy
