----- Original Message ----- > Thank you for your comment! > > I've uploaded new webrev: > http://cr.openjdk.java.net/~ysuenaga/JDK-8036003/webrev.01/ > > Could you review it again? >
This looks better :) You could move the default setting into the AC_ARG_WITH itself (the fourth argument is 'action-if-not-given' [0]), as I did below, but it's not a huge thing. [0] https://www.gnu.org/software/autoconf/manual/autoconf-2.60/html_node/External-Software.html > > Thanks, > > Yasumasa > > > On 2015/11/17 0:51, Andrew Hughes wrote: > > > > ----- Original Message ----- > >> Hello, > >> > >> I think this looks mostly OK, but I'm pretty sure Magnus will want to > >> comment. I'm afraid he likely won't be able to until next week however. > >> > >> One thing, could you name the variable DEBUG_SYMBOLS, with the S at the > >> end so that it matches the configure option? > >> > >> /Erik > >> > >> On 2015-11-16 10:38, Yasumasa Suenaga wrote: > >>> Hi all, > >>> > >>> I would like to propose JDK-8036003: Add > >>> --with-debug-symbols=[none|internal|external|zipped] . > >>> > >>> I had posted this enhancement which provides new make variable [1]. > >>> But it does not fit current jdk 9. So I propose it again as new configure > >>> option. > >>> > >>> I'm sure that this enhancement helps Linux distros to package OpenJDK. > >>> For example, if you create RPM package of OpenJDK, you have to pass many > >>> make variables [2]. > >>> > >>> Could you review it? > >>> http://cr.openjdk.java.net/~ysuenaga/JDK-8036003/webrev.00/ > >>> > >>> > >>> Thanks, > >>> > >>> Yasumasa > >>> > >>> > >>> [1] > >>> http://mail.openjdk.java.net/pipermail/build-dev/2014-February/012019.html > >>> [2] > >>> http://mail.openjdk.java.net/pipermail/distro-pkg-dev/2015-November/034036.html > >>> > >> > >> > > > > It looks mostly ok to me too. It would be good to finally get this > > simplified. > > > > I agree on the pluralisation, but the problem there is the existing badly > > named ENABLE_DEBUG_SYMBOLS which actually seems to enable splitting > > debuginfo, > > going by this proposed change. Technically, I don't think the two conflict, > > but > > it is potentially (even more) confusing to read. > > > > The message AC_MSG_CHECKING([type of debug symbol]) should definitely be > > changed to the plural. I suggest AC_MSG_CHECKING([what type of debug > > symbols to use]). > > Also, AC_MSG_RESULT should precede the error check so the user can see the > > bad > > value that is failing: > > > > + AC_MSG_CHECKING([what type of debug symbols to use]) > > + AC_ARG_WITH([debug-symbols], [AS_HELP_STRING([--with-debug-symbols], > > + [set the debug symbol configuration (none, internal, external, > > zipped) @<:@zipped@:>@])], > > + [ > > + DEBUG_SYMBOL="${withval}" > > + ], > > + [ > > + DEBUG_SYMBOL="zipped" > > + ]) > > + AC_MSG_RESULT([$DEBUG_SYMBOL]) > > + > > + if test "x$DEBUG_SYMBOL" != xzipped && \ > > + test "x$DEBUG_SYMBOL" != xnone && \ > > + test "x$DEBUG_SYMBOL" != xinternal && \ > > + test "x$DEBUG_SYMBOL" != xexternal; then > > + AC_MSG_ERROR([Allowed debug symbols are: none, internal, external, > > zipped]) > > + fi > > + > > > -- Andrew :) Senior Free Java Software Engineer Red Hat, Inc. (http://www.redhat.com) PGP Key: ed25519/35964222 (hkp://keys.gnupg.net) Fingerprint = 5132 579D D154 0ED2 3E04 C5A0 CFDA 0F9B 3596 4222 PGP Key: rsa4096/248BDC07 (hkp://keys.gnupg.net) Fingerprint = EC5A 1F5E C0AD 1D15 8F1F 8F91 3B96 A578 248B DC07
