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 >>>
