On 2016-02-10 07:13, Tim Bell wrote:
Erik:
Please review these fixes for compare.sh and the COMPARE_BUILD flag
for make.
The rather new feature COMPARE_BUILD, which builds twice, applying
some kind of change between them, is really neat. Especially when run
through JPRT to check all platforms at once. The problem is that
compare.sh currently isn't clean on all these platforms. There are
also some special peculiarities particular to when JPRT is running
the build which confuses compare.sh.
Given that COMPARE_BUILD uses the exact same output directory, and
JPRT sets the version-opt string to a fix value for both builds, we
should be able to achieve a clean and stable baseline for an empty
patch file. Which I now have.
I also changed COMPARE_BUILD to fail the build when differences are
found and added an option to COMPARE_BUILD to disable failing.
Bug: https://bugs.openjdk.java.net/browse/JDK-8149479
Webrev: http://cr.openjdk.java.net/~erikj/8149479/webrev.01/
1) Regarding common/bin/compare.sh lines 336...341 and lines 344...349
in the new version - this is not directly related to 8149479 but I
wish those regexes were only coded once. If they ever get out of sync
the results will be puzzling.
2) make/InitSupport.gmk line 366 in the new version - typo?
COMPARE_BUILD_FAILE is COMPARE_BUILD_FAIL elsewhere
Also, the comment lists an additional "FAIL_BUILD=<bool>" which should
be deleted.
Apart from that (and the typo Tim found), the patch looks good to me.
However, I'd like to use the opportunity to talk a bit about the compare
script. :-)
I think it is a very good initiative to fix the compare script so that
two consequitive builds on the same machine should compare equal.
Apparently, that's as close to a reproducible build we'll ever get at
the moment.
This patch does that, and that's good.
However, the "naive" way to get to a clean compare is to stop comparing
anything of value. :-) I'm not suggesting that you are doing that (or
that you should do that), but in a less extreme form, that's my main
worry. Or to phrase it differently, how can we tell that we're not
hiding something important? For instance, all these "accepted size
differences" on solaris. Are they still relevant?
Is there some way to run the compare script without the exceptions file
(apart from hacking it)? If we do that, what would happen? Could we trim
down the exception lists?
The dis filters seems a bit non-optimal too. We have a "global" dis
filter in compare.sh (introduced by me when I didn't know about the
specific dis filters in the exception file), and a specific dis filter
per platform in the exception file. The main task for the dis filters is
to remove differences in hexadecimal numbers, and I think that could be
generalized enough to just be in the global part. Any highly platform
specific filtering, like the "literal pool" stuff for macosx, could
still live in the exception file.
Finally, I still would like to get the string literal comparison into
the configure script, now I run it manually using:
objdump -s -j .rodata $LIBRARY | grep "^ " | xxd -r | strings
(The main hurdle to do this is adding detection for xxd in configure, I
think)
/Magnus
