+1
Thanks Vladimir > On Jan 18, 2019, at 5:29 PM, Igor Ignatev <igor.ignat...@oracle.com> wrote: > > Still looks good to me. > > — Igor > >> On Jan 18, 2019, at 5:26 PM, Vladimir Ivanov <vladimir.x.iva...@oracle.com> >> wrote: >> >> Updated webrev: >> http://cr.openjdk.java.net/~vlivanov/8217404/webrev.01 >> >> Verified that it works as expected on Linux, Windows, MacOS, and Solaris. >> >> Best regards, >> Vladimir Ivanov >> >>>> On 18/01/2019 16:39, Vladimir Ivanov wrote: >>>> Thanks, Igor. >>>> overall your fix looks reasonable, but w/ it we can get unintentionally >>>> disabled features (b/c grep doesn't do full word match). although this >>>> problem wasn't really introduced by your fix, I think it's be better to >>>> fix it as a part of your patch. I see two possible solutions: >>> I was aware of such drawback, but decided to leave it as is, since it >>> doesn't affect existing features. >>>> - add "-w" to grep, but I am not sure if "-w" is supported by all grep >>>> implementations >>>> - use $XARGS instead of $ECHO when we get DISABLE_X. in this case you >>>> will need to revert your changes in 'if test ...' lines >>> I'm in favor of using "-w" and I see different grep flags being used >>> already, but would like somebody from Build team confirm they are OK with >>> such solution. >>> Best regards, >>> Vladimir Ivanov >>>>> On Jan 18, 2019, at 3:33 PM, Vladimir Ivanov >>>>> <vladimir.x.iva...@oracle.com> wrote: >>>>> >>>>> http://cr.openjdk.java.net/~vlivanov/8217404/webrev.00/ >>>>> https://bugs.openjdk.java.net/browse/JDK-8217404 >>>>> >>>>> --with-jvm-features doesn't work properly when multiple features are >>>>> explicitly disabled: >>>>> >>>>> $ bash configure --with-jvm-features="-aot -jvmci -graal" >>>>> ... >>>>> checking if jvmci module jdk.internal.vm.ci should be built... yes >>>>> checking if graal module jdk.internal.vm.compiler should be built... yes >>>>> checking if aot should be enabled... yes >>>>> ... >>>>> >>>>> The problem in the following code: >>>>> >>>>> DISABLE_AOT=`$ECHO $DISABLED_JVM_FEATURES | $GREP aot` >>>>> if test "x$DISABLE_AOT" = "xaot"; then >>>>> ENABLE_AOT="false" >>>>> fi >>>>> >>>>> Since DISABLED_JVM_FEATURES ("aot jvmci graal") contains the list of >>>>> explicitly disabled features, grep over it returns the whole list when >>>>> there's a match. The subsequent check fails because there's no exact >>>>> match, though DISABLE_AOT contains "aot" . >>>>> >>>>> Proposed fix is to check there's no match instead. >>>>> >>>>> After the fix it works as expected: >>>>> >>>>> $ bash configure --with-jvm-features="-aot -jvmci -graal" >>>>> ... >>>>> checking if jvmci module jdk.internal.vm.ci should be built... no, forced >>>>> checking if graal module jdk.internal.vm.compiler should be built... no, >>>>> forced >>>>> checking if aot should be enabled... no, forced >>>>> ... >>>>> >>>>> (The fix doesn't address the case when one feature has a name which is a >>>>> proper substring of another feature, but there are no such cases at the >>>>> moment.) >>>>> >>>>> Best regards, >>>>> Vladimir Ivanov >>>> >
