Fix setting default values for Pattern options
Options in bazel can have default values assigned to them, which are validated
against allowed values. We have a bug where restricting the possible values of
a Pattern will break the default value. It happens because we create
Pattern objects, which use the default (by reference) implementation of equals.
This violates the contract used by bazel converters that options created from
the same representation will return true if compared using corresponding equals
method.
Create a wrapper around Pattern which implements proper equals (compliant with
the contract). Also, specify the contract in Converter to explicitly state the
assumptions and add tests covering the RegexPatternConverter and the
problematic scenario from the bug.
RELNOTES: none.
PiperOrigin-RevId: 241764234
diff --git a/src/main/java/com/google/devtools/build/lib/BUILD b/src/main/java/com/google/devtools/build/lib/BUILD
index fee3fcd..119de1b 100644
--- a/src/main/java/com/google/devtools/build/lib/BUILD
+++ b/src/main/java/com/google/devtools/build/lib/BUILD
@@ -1199,6 +1199,7 @@
"//src/main/java/com/google/devtools/build/lib/vfs:pathfragment",
"//src/main/java/com/google/devtools/common/options",
"//third_party:guava",
+ "//third_party:jsr305",
],
)
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/ExecutionInfoModifier.java b/src/main/java/com/google/devtools/build/lib/analysis/config/ExecutionInfoModifier.java
index ff37fb0..3777ffc 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/config/ExecutionInfoModifier.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/config/ExecutionInfoModifier.java
@@ -19,7 +19,6 @@
import com.google.common.base.Splitter;
import com.google.common.base.Strings;
import com.google.common.collect.ImmutableList;
-import com.google.common.collect.ImmutableList.Builder;
import com.google.devtools.common.options.Converters.RegexPatternConverter;
import com.google.devtools.common.options.OptionsParsingException;
import java.util.Map;
@@ -77,7 +76,10 @@
new AutoValue_ExecutionInfoModifier_Expression(
// Convert to get a useful exception if it's not a valid pattern, but use the regex
// (see comment in Expression)
- new RegexPatternConverter().convert(specMatcher.group("pattern")).pattern(),
+ new RegexPatternConverter()
+ .convert(specMatcher.group("pattern"))
+ .regexPattern()
+ .pattern(),
specMatcher.group("sign").equals("-"),
specMatcher.group("key")));
}
diff --git a/src/main/java/com/google/devtools/build/lib/buildtool/BuildRequestOptions.java b/src/main/java/com/google/devtools/build/lib/buildtool/BuildRequestOptions.java
index 79845b2..68f3197 100644
--- a/src/main/java/com/google/devtools/build/lib/buildtool/BuildRequestOptions.java
+++ b/src/main/java/com/google/devtools/build/lib/buildtool/BuildRequestOptions.java
@@ -28,9 +28,10 @@
import com.google.devtools.common.options.OptionsBase;
import com.google.devtools.common.options.OptionsParser;
import com.google.devtools.common.options.OptionsParsingException;
+import com.google.devtools.common.options.RegexPatternOption;
import java.util.List;
import java.util.logging.Logger;
-import java.util.regex.Pattern;
+import javax.annotation.Nullable;
/**
* Options interface for {@link BuildRequest}: can be used to parse command-line arguments.
@@ -100,14 +101,14 @@
public boolean verboseExplanations;
@Option(
- name = "output_filter",
- converter = Converters.RegexPatternConverter.class,
- defaultValue = "null",
- documentationCategory = OptionDocumentationCategory.LOGGING,
- effectTags = {OptionEffectTag.AFFECTS_OUTPUTS},
- help = "Only shows warnings for rules with a name matching the provided regular expression."
- )
- public Pattern outputFilter;
+ name = "output_filter",
+ converter = Converters.RegexPatternConverter.class,
+ defaultValue = "null",
+ documentationCategory = OptionDocumentationCategory.LOGGING,
+ effectTags = {OptionEffectTag.AFFECTS_OUTPUTS},
+ help = "Only shows warnings for rules with a name matching the provided regular expression.")
+ @Nullable
+ public RegexPatternOption outputFilter;
@Option(
name = "analyze",
@@ -189,15 +190,15 @@
public boolean announce;
@Option(
- name = "symlink_prefix",
- defaultValue = "null",
- documentationCategory = OptionDocumentationCategory.OUTPUT_PARAMETERS,
- effectTags = {OptionEffectTag.AFFECTS_OUTPUTS},
- help =
- "The prefix that is prepended to any of the convenience symlinks that are created "
- + "after a build. If '/' is passed, then no symlinks are created and no warning is "
- + "emitted. If omitted, the default value is the name of the build tool."
- )
+ name = "symlink_prefix",
+ defaultValue = "null",
+ documentationCategory = OptionDocumentationCategory.OUTPUT_PARAMETERS,
+ effectTags = {OptionEffectTag.AFFECTS_OUTPUTS},
+ help =
+ "The prefix that is prepended to any of the convenience symlinks that are created "
+ + "after a build. If '/' is passed, then no symlinks are created and no warning is "
+ + "emitted. If omitted, the default value is the name of the build tool.")
+ @Nullable
public String symlinkPrefix;
@Option(
diff --git a/src/main/java/com/google/devtools/build/lib/buildtool/BuildTool.java b/src/main/java/com/google/devtools/build/lib/buildtool/BuildTool.java
index 10f0f6a..c00274d 100644
--- a/src/main/java/com/google/devtools/build/lib/buildtool/BuildTool.java
+++ b/src/main/java/com/google/devtools/build/lib/buildtool/BuildTool.java
@@ -46,9 +46,9 @@
import com.google.devtools.build.lib.util.ExitCode;
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.common.options.OptionsProvider;
+import com.google.devtools.common.options.RegexPatternOption;
import java.util.logging.Level;
import java.util.logging.Logger;
-import java.util.regex.Pattern;
/**
* Provides the bulk of the implementation of the 'blaze build' command.
@@ -342,9 +342,11 @@
* Initializes the output filter to the value given with {@code --output_filter}.
*/
private void initializeOutputFilter(BuildRequest request) {
- Pattern outputFilter = request.getBuildOptions().outputFilter;
- if (outputFilter != null) {
- getReporter().setOutputFilter(OutputFilter.RegexOutputFilter.forPattern(outputFilter));
+ RegexPatternOption outputFilterOption = request.getBuildOptions().outputFilter;
+ if (outputFilterOption != null) {
+ getReporter()
+ .setOutputFilter(
+ OutputFilter.RegexOutputFilter.forPattern(outputFilterOption.regexPattern()));
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/exec/local/LocalExecutionOptions.java b/src/main/java/com/google/devtools/build/lib/exec/local/LocalExecutionOptions.java
index d1eabcc..32e1cee 100644
--- a/src/main/java/com/google/devtools/build/lib/exec/local/LocalExecutionOptions.java
+++ b/src/main/java/com/google/devtools/build/lib/exec/local/LocalExecutionOptions.java
@@ -18,8 +18,8 @@
import com.google.devtools.common.options.OptionDocumentationCategory;
import com.google.devtools.common.options.OptionEffectTag;
import com.google.devtools.common.options.OptionsBase;
+import com.google.devtools.common.options.RegexPatternOption;
import java.time.Duration;
-import java.util.regex.Pattern;
/**
* Local execution options.
@@ -39,16 +39,15 @@
public int localSigkillGraceSeconds;
@Option(
- name = "allowed_local_actions_regex",
- documentationCategory = OptionDocumentationCategory.UNDOCUMENTED,
- effectTags = {OptionEffectTag.UNKNOWN},
- converter = Converters.RegexPatternConverter.class,
- defaultValue = "null",
- help =
- "A regex whitelist for action types which may be run locally. If unset, "
- + "all actions are allowed to execute locally"
- )
- public Pattern allowedLocalAction;
+ name = "allowed_local_actions_regex",
+ documentationCategory = OptionDocumentationCategory.UNDOCUMENTED,
+ effectTags = {OptionEffectTag.UNKNOWN},
+ converter = Converters.RegexPatternConverter.class,
+ defaultValue = "null",
+ help =
+ "A regex whitelist for action types which may be run locally. If unset, "
+ + "all actions are allowed to execute locally")
+ public RegexPatternOption allowedLocalAction;
@Option(
name = "experimental_collect_local_action_metrics",
diff --git a/src/main/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunner.java b/src/main/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunner.java
index eb92c7c..4426ad4 100644
--- a/src/main/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunner.java
+++ b/src/main/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunner.java
@@ -254,11 +254,21 @@
FileOutErr outErr = context.getFileOutErr();
String actionType = spawn.getResourceOwner().getMnemonic();
if (localExecutionOptions.allowedLocalAction != null
- && !localExecutionOptions.allowedLocalAction.matcher(actionType).matches()) {
+ && !localExecutionOptions
+ .allowedLocalAction
+ .regexPattern()
+ .matcher(actionType)
+ .matches()) {
setState(State.PERMANENT_ERROR);
- outErr.getErrorStream().write(
- ("Action type " + actionType + " is not allowed to run locally due to regex filter: "
- + localExecutionOptions.allowedLocalAction + "\n").getBytes(UTF_8));
+ outErr
+ .getErrorStream()
+ .write(
+ ("Action type "
+ + actionType
+ + " is not allowed to run locally due to regex filter: "
+ + localExecutionOptions.allowedLocalAction.regexPattern()
+ + "\n")
+ .getBytes(UTF_8));
return new SpawnResult.Builder()
.setRunnerName(getName())
.setStatus(Status.EXECUTION_DENIED)
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/commands/ProfileCommand.java b/src/main/java/com/google/devtools/build/lib/runtime/commands/ProfileCommand.java
index 8590ea4..5a3330a 100644
--- a/src/main/java/com/google/devtools/build/lib/runtime/commands/ProfileCommand.java
+++ b/src/main/java/com/google/devtools/build/lib/runtime/commands/ProfileCommand.java
@@ -44,6 +44,7 @@
import com.google.devtools.common.options.OptionsBase;
import com.google.devtools.common.options.OptionsParser;
import com.google.devtools.common.options.OptionsParsingResult;
+import com.google.devtools.common.options.RegexPatternOption;
import java.io.BufferedOutputStream;
import java.io.IOException;
import java.io.PrintStream;
@@ -153,15 +154,15 @@
public boolean htmlHistograms;
@Option(
- name = "task_tree",
- defaultValue = "null",
- converter = Converters.RegexPatternConverter.class,
- documentationCategory = OptionDocumentationCategory.LOGGING,
- effectTags = {OptionEffectTag.AFFECTS_OUTPUTS},
- help =
- "Print the tree of profiler tasks from all tasks matching the given regular expression."
- )
- public Pattern taskTree;
+ name = "task_tree",
+ defaultValue = "null",
+ converter = Converters.RegexPatternConverter.class,
+ documentationCategory = OptionDocumentationCategory.LOGGING,
+ effectTags = {OptionEffectTag.AFFECTS_OUTPUTS},
+ help =
+ "Print the tree of profiler tasks from all tasks matching the given regular"
+ + " expression.")
+ public RegexPatternOption taskTree;
@Option(
name = "task_tree_threshold",
@@ -272,7 +273,7 @@
}
if (opts.taskTree != null) {
- printTaskTree(out, name, info, opts.taskTree, opts.taskTreeThreshold);
+ printTaskTree(out, name, info, opts.taskTree.regexPattern(), opts.taskTreeThreshold);
continue;
}
diff --git a/src/main/java/com/google/devtools/common/options/BUILD b/src/main/java/com/google/devtools/common/options/BUILD
index f055d4b..5936b2c 100644
--- a/src/main/java/com/google/devtools/common/options/BUILD
+++ b/src/main/java/com/google/devtools/common/options/BUILD
@@ -34,6 +34,7 @@
],
),
deps = [
+ "//third_party:auto_value",
"//third_party:guava",
"//third_party:jsr305",
],
diff --git a/src/main/java/com/google/devtools/common/options/Converter.java b/src/main/java/com/google/devtools/common/options/Converter.java
index 9b3c13b..f3e6ddd 100644
--- a/src/main/java/com/google/devtools/common/options/Converter.java
+++ b/src/main/java/com/google/devtools/common/options/Converter.java
@@ -20,7 +20,8 @@
public interface Converter<T> {
/**
- * Convert a string into type T.
+ * Convert a string into type T. Please note that we assume that converting the same string (if
+ * successful) will produce objects which are equal ({@link Object#equals)}).
*/
T convert(String input) throws OptionsParsingException;
diff --git a/src/main/java/com/google/devtools/common/options/Converters.java b/src/main/java/com/google/devtools/common/options/Converters.java
index 6874c7e..ef62c9b 100644
--- a/src/main/java/com/google/devtools/common/options/Converters.java
+++ b/src/main/java/com/google/devtools/common/options/Converters.java
@@ -355,12 +355,12 @@
}
/** Checks whether a string is a valid regex pattern and compiles it. */
- public static class RegexPatternConverter implements Converter<Pattern> {
+ public static class RegexPatternConverter implements Converter<RegexPatternOption> {
@Override
- public Pattern convert(String input) throws OptionsParsingException {
+ public RegexPatternOption convert(String input) throws OptionsParsingException {
try {
- return Pattern.compile(input);
+ return RegexPatternOption.create(Pattern.compile(input));
} catch (PatternSyntaxException e) {
throw new OptionsParsingException("Not a valid regular expression: " + e.getMessage());
}
diff --git a/src/main/java/com/google/devtools/common/options/RegexPatternOption.java b/src/main/java/com/google/devtools/common/options/RegexPatternOption.java
new file mode 100644
index 0000000..bdf6313
--- /dev/null
+++ b/src/main/java/com/google/devtools/common/options/RegexPatternOption.java
@@ -0,0 +1,54 @@
+// Copyright 2019 The Bazel Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+package com.google.devtools.common.options;
+
+import com.google.auto.value.AutoValue;
+import com.google.common.base.Preconditions;
+import java.util.regex.Pattern;
+
+/**
+ * Option class wrapping a {@link Pattern class}. We wrap the {@link Pattern} class instance since
+ * it uses reference equality, which breaks the assumption of {@link Converter} that {@code
+ * converter.convert(sameString).equals(converter.convert(sameString)}.
+ *
+ * <p>Please note that the equality implementation is based solely on the input regex, therefore
+ * patterns expressing the same intent with different regular expressions (e.g. {@code "a"} and
+ * {@code "[a]"} will not be treated as equal.
+ */
+@AutoValue
+public abstract class RegexPatternOption {
+ static RegexPatternOption create(Pattern regexPattern) {
+ return new AutoValue_RegexPatternOption(Preconditions.checkNotNull(regexPattern));
+ }
+
+ public abstract Pattern regexPattern();
+
+ @Override
+ public final boolean equals(Object other) {
+ if (this == other) {
+ return true;
+ }
+ if (!(other instanceof RegexPatternOption)) {
+ return false;
+ }
+
+ RegexPatternOption otherOption = (RegexPatternOption) other;
+ return otherOption.regexPattern().pattern().equals(regexPattern().pattern());
+ }
+
+ @Override
+ public final int hashCode() {
+ return regexPattern().pattern().hashCode();
+ }
+}
diff --git a/src/test/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunnerTest.java b/src/test/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunnerTest.java
index 4d1f118..3a85428 100644
--- a/src/test/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunnerTest.java
+++ b/src/test/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunnerTest.java
@@ -63,6 +63,7 @@
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.PathFragment;
import com.google.devtools.build.lib.vfs.inmemoryfs.InMemoryFileSystem;
+import com.google.devtools.common.options.Converters.RegexPatternConverter;
import com.google.devtools.common.options.Options;
import java.io.ByteArrayInputStream;
import java.io.File;
@@ -78,7 +79,6 @@
import java.util.logging.Filter;
import java.util.logging.LogRecord;
import java.util.logging.Logger;
-import java.util.regex.Pattern;
import org.junit.Before;
import org.junit.Test;
import org.junit.runner.RunWith;
@@ -552,7 +552,7 @@
FileSystem fs = setupEnvironmentForFakeExecution();
LocalExecutionOptions options = Options.getDefaults(LocalExecutionOptions.class);
- options.allowedLocalAction = Pattern.compile("none");
+ options.allowedLocalAction = new RegexPatternConverter().convert("none");
LocalSpawnRunner runner =
new TestedLocalSpawnRunner(
fs.getPath("/execroot"),
diff --git a/src/test/java/com/google/devtools/common/options/BUILD b/src/test/java/com/google/devtools/common/options/BUILD
index 76d843b..40fe246 100644
--- a/src/test/java/com/google/devtools/common/options/BUILD
+++ b/src/test/java/com/google/devtools/common/options/BUILD
@@ -43,6 +43,7 @@
"//src/main/java/com/google/devtools/build/lib:util",
"//src/main/java/com/google/devtools/common/options",
"//src/main/java/com/google/devtools/common/options:invocation_policy",
+ "//src/main/java/com/google/devtools/common/options/testing",
"//src/main/protobuf:invocation_policy_java_proto",
"//src/main/protobuf:option_filters_java_proto",
"//src/test/java/com/google/devtools/build/lib:testutil",
diff --git a/src/test/java/com/google/devtools/common/options/RegexPatternConverterTest.java b/src/test/java/com/google/devtools/common/options/RegexPatternConverterTest.java
new file mode 100644
index 0000000..7d86514
--- /dev/null
+++ b/src/test/java/com/google/devtools/common/options/RegexPatternConverterTest.java
@@ -0,0 +1,67 @@
+// Copyright 2019 The Bazel Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+package com.google.devtools.common.options;
+
+import static com.google.common.truth.Truth.assertThat;
+import static org.junit.Assert.fail;
+
+import com.google.devtools.common.options.Converters.RegexPatternConverter;
+import com.google.devtools.common.options.testing.ConverterTester;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.JUnit4;
+
+/** A test for {@link RegexPatternConverter} */
+@RunWith(JUnit4.class)
+public class RegexPatternConverterTest {
+ @Test
+ public void consistentEqualsAndHashCodeForSamePattern() {
+ new ConverterTester(RegexPatternConverter.class)
+ .addEqualityGroup("")
+ .addEqualityGroup(".*")
+ .addEqualityGroup("[^\\s]+")
+ .testConvert();
+ }
+
+ @Test
+ public void comparisonBasedOnInputOnly() {
+ String regex = "a";
+ String semanticallyTheSame = "[a]";
+
+ new ConverterTester(RegexPatternConverter.class)
+ .addEqualityGroup(regex)
+ .addEqualityGroup(semanticallyTheSame)
+ .testConvert();
+ }
+
+ @Test
+ public void createsProperPattern() throws OptionsParsingException {
+ RegexPatternConverter converter = new RegexPatternConverter();
+ for (String regex : new String[] {"", ".*", "\\s*(\\w+)", "prefix (suffix1|suffix2)"}) {
+ // We are not testing {@link Pattern} itself -- the assumption is that if {@link
+ // Pattern#pattern} returns the proper string, we created the right pattern.
+ assertThat(converter.convert(regex).regexPattern().pattern()).isEqualTo(regex);
+ }
+ }
+
+ @Test
+ public void throwsForWrongPattern() {
+ try {
+ new RegexPatternConverter().convert("{");
+ fail();
+ } catch (OptionsParsingException e) {
+ assertThat(e).hasMessageThat().startsWith("Not a valid regular expression:");
+ }
+ }
+}
