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
