On 2/12/19 12:05 PM, Severin Gehwolf wrote:
Hi Mandy,
Thanks for the review!
OK. Here is the summary:
http://mail.openjdk.java.net/pipermail/jigsaw-dev/2019-February/014132.html
Personally, I find --strip-native-debug-info or --strip-native-debug-
symbols clearer than just --strip-native-debug.
Fine with me and we can stick with --strip-native-debug-symbols
W.r.t. the test:
> The test currently only runs on Linux
> and requires objcopy to be available. Otherwise the test is being
> skipped.
We can create a fake objcopy utility for testing purpose
such that the test will validate if the options are passed
properly to the test utility.
AFAIK, objcopy is a build requirement for OpenJDK already, so I'm
wondering whether it makes sense to stub objcopy for the tests. Perhaps
you meant that to be used in addition to existing tests?
Test machines may not have objcopy. It'd increase the test
coverage to include a test that can run on all test machines.
Yes it's an additional test in addition to a test you have
that only runs if objcopy utility is present.
Anyhow, I'll look into it. It'll likely have to use the '--strip-
native-debug-symbols objcopy=<path/to/fake-objcopy' option in that
case.
What I was thinking is:
--strip-native-debug-symbols objcopy="java fakeobjcopy"
where objcopy accepts a command that can contain arguments.
It'll have to be some native code which will require some custom
make machinery to get compiled, no? If you have pointers to examples in
the JDK already, I'd appreciate it.
See the idea above.
> webrev:
http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8214796/04/webrev/
I haven't reviewed the new files. Just notice that very long
lines in the new files that you may want to fix the formatting
that will help further side-by-side review.
Some of the longer lines have been cleaned up in the lates webrev:
http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8214796/05/webrev/
The --list-plugin output is very very long. The existing plugins
didn't set a good example to keep the well formatted (I recorded
that they were cleaned up at one point to keep 80 chars width).
Right. I'll make sure --list-plugins looks right for --strip-native-
debug-symbols once we've agreed on the options.
One other question: should this plugin be moved to linux/classes
rather than unix/classes given that that's the platform it
intends to support?
Done in version 05.
Thanks for making the change. I will reply the other thread.
Mandy
