Add --all_incompatible_changes, the user's shorthand for turning on all --incompatible_* flags
Note that if a developer adds a poorly-formatted incompatible change @Option, constructing an OptionsParser will now fail with an unchecked exception. This can cause some unit tests to fail in unexpected ways, but the developer should see an appropriate error message when the server starts up.
To be added: A separate integration test that ensures the expansions of --all_incompatible_changes don't clobber each other.
RELNOTES: None
PiperOrigin-RevId: 151858287
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/AllIncompatibleChangesExpansion.java b/src/main/java/com/google/devtools/build/lib/runtime/AllIncompatibleChangesExpansion.java
new file mode 100644
index 0000000..3a32412
--- /dev/null
+++ b/src/main/java/com/google/devtools/build/lib/runtime/AllIncompatibleChangesExpansion.java
@@ -0,0 +1,161 @@
+// Copyright 2017 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.build.lib.runtime;
+
+import com.google.common.collect.Iterables;
+import com.google.devtools.common.options.Converter;
+import com.google.devtools.common.options.ExpansionFunction;
+import com.google.devtools.common.options.IsolatedOptionsData;
+import com.google.devtools.common.options.Option;
+import com.google.devtools.common.options.OptionsBase;
+import com.google.devtools.common.options.OptionsParser;
+import java.lang.reflect.Field;
+import java.util.ArrayList;
+import java.util.Map;
+
+/**
+ * Expansion function for {@code --all_incompatible_changes}. Expands to all options of form {@code
+ * --incompatible_*} that are declared in the {@link OptionsBase} subclasses that are passed to the
+ * parser.
+ *
+ * <p>The incompatible changes system provides users with a uniform way of opting into backwards-
+ * incompatible changes, in order to test whether their builds will be broken by an upcoming
+ * release. When adding a new breaking change to Bazel, prefer to use this mechanism for guarding
+ * the behavior.
+ *
+ * <p>An {@link Option}-annotated field that is considered an incompatible change must satisfy the
+ * following requirements.
+ *
+ * <ul>
+ * <li>the {@link Option#name} must be prefixed with "incompatible_"
+ * <li>the {@link Option#category} must be "incompatible changes"
+ * <li>the {@link Option#help} field must be set, and must refer the user to information about
+ * what the change does and how to migrate their code
+ * <li>the following fields may not be used: {@link Option#abbrev}, {@link Option#valueHelp},
+ * {@link Option#converter}, {@link Option#allowMultiple}, {@link Option#oldName}, and {@link
+ * Option#wrapperOption}
+ * </ul>
+ *
+ * Example:
+ *
+ * <pre>{@code
+ * @Option(
+ * name = "incompatible_foo",
+ * category = "incompatible changes",
+ * defaultValue = "false",
+ * help = "Deprecates bar and changes the semantics of baz. To migrate your code see [...].")
+ * public boolean incompatibleFoo;
+ * }</pre>
+ *
+ * All options that satisfy either the name or category requirement will be validated using the
+ * above criteria. Any failure will cause {@link IllegalArgumentException} to be thrown, which will
+ * cause the construction of the {@link OptionsParser} to fail with the <i>unchecked</i> exception
+ * {@link OptionsParser.ConstructionException}. Therefore, when adding a new incompatible change, be
+ * aware that an error in the specification of the {@code @Option} will exercise failure code paths
+ * in the early part of the Bazel server execution.
+ *
+ * <p>After the breaking change has been enabled unconditionally, it is recommended (required?) that
+ * its corresponding incompatible change option be left as a valid no-op option, rather than
+ * removed. This helps avoid breaking invocations of Bazel upon upgrading to a new release. Just as
+ * for other options, names of incompatible change options must never be reused for a different
+ * option.
+ */
+// Javadoc can't resolve inner classes.
+@SuppressWarnings("javadoc")
+public class AllIncompatibleChangesExpansion implements ExpansionFunction {
+
+ // The reserved prefix for all incompatible change option names.
+ public static final String INCOMPATIBLE_NAME_PREFIX = "incompatible_";
+ // The reserved category for all incompatible change options.
+ public static final String INCOMPATIBLE_CATEGORY = "incompatible changes";
+
+ /**
+ * Ensures that the given option satisfies all the requirements on incompatible change options
+ * enumerated above.
+ *
+ * <p>If any of these requirements are not satisfied, {@link IllegalArgumentException} is thrown,
+ * as this constitutes an internal error in the declaration of the option.
+ */
+ private static void validateIncompatibleChange(Field field, Option annotation) {
+ String prefix = "Incompatible change option '--" + annotation.name() + "' ";
+
+ // To avoid ambiguity, and the suggestion of using .isEmpty().
+ String defaultString = "";
+
+ // Validate that disallowed fields aren't used. These will need updating if the default values
+ // in Option ever change, and perhaps if new fields are added.
+ if (annotation.abbrev() != '\0') {
+ throw new IllegalArgumentException(prefix + "must not use the abbrev field");
+ }
+ if (!annotation.valueHelp().equals(defaultString)) {
+ throw new IllegalArgumentException(prefix + "must not use the valueHelp field");
+ }
+ if (annotation.converter() != Converter.class) {
+ throw new IllegalArgumentException(prefix + "must not use the converter field");
+ }
+ if (annotation.allowMultiple()) {
+ throw new IllegalArgumentException(prefix + "must not use the allowMultiple field");
+ }
+ if (annotation.implicitRequirements().length > 0) {
+ throw new IllegalArgumentException(prefix + "must not use the implicitRequirements field");
+ }
+ if (!annotation.oldName().equals(defaultString)) {
+ throw new IllegalArgumentException(prefix + "must not use the oldName field");
+ }
+ if (annotation.wrapperOption()) {
+ throw new IllegalArgumentException(prefix + "must not use the wrapperOption field");
+ }
+
+ // Validate the fields that are actually allowed.
+ if (!annotation.name().startsWith(INCOMPATIBLE_NAME_PREFIX)) {
+ throw new IllegalArgumentException(prefix + "must have name starting with \"incompatible_\"");
+ }
+ if (!annotation.category().equals(INCOMPATIBLE_CATEGORY)) {
+ throw new IllegalArgumentException(prefix + "must have category \"incompatible changes\"");
+ }
+ if (!IsolatedOptionsData.isExpansionOption(annotation)) {
+ if (!field.getType().equals(Boolean.TYPE)) {
+ throw new IllegalArgumentException(
+ prefix + "must have boolean type (unless it's an expansion option)");
+ }
+ }
+ if (annotation.help().equals(defaultString)) {
+ throw new IllegalArgumentException(
+ prefix
+ + "must have a \"help\" string that refers the user to "
+ + "information about this change and how to migrate their code");
+ }
+ }
+
+ @Override
+ public String[] getExpansion(IsolatedOptionsData optionsData) {
+ // Grab all registered options that are identified as incompatible changes by either name or
+ // by category. Ensure they satisfy our requirements.
+ ArrayList<String> incompatibleChanges = new ArrayList<>();
+ for (Map.Entry<String, Field> entry : optionsData.getAllNamedFields()) {
+ Field field = entry.getValue();
+ Option annotation = field.getAnnotation(Option.class);
+ if (annotation.name().startsWith(INCOMPATIBLE_NAME_PREFIX)
+ || annotation.category().equals(INCOMPATIBLE_CATEGORY)) {
+ validateIncompatibleChange(field, annotation);
+ incompatibleChanges.add("--" + annotation.name());
+ }
+ }
+ // Sort to get a deterministic canonical order. This probably isn't necessary because the
+ // options parser will do its own sorting when canonicalizing, but it seems like it can't hurt.
+ incompatibleChanges.sort(null);
+ return Iterables.toArray(incompatibleChanges, String.class);
+ }
+}
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/CommonCommandOptions.java b/src/main/java/com/google/devtools/build/lib/runtime/CommonCommandOptions.java
index 2afb774..8e6cc14 100644
--- a/src/main/java/com/google/devtools/build/lib/runtime/CommonCommandOptions.java
+++ b/src/main/java/com/google/devtools/build/lib/runtime/CommonCommandOptions.java
@@ -97,6 +97,18 @@
}
+ // To create a new incompatible change, see the javadoc for AllIncompatibleChangesExpansion.
+ @Option(
+ name = "all_incompatible_changes",
+ defaultValue = "null",
+ category = "misc",
+ expansionFunction = AllIncompatibleChangesExpansion.class,
+ help =
+ "Enables all options of the form --incompatible_*. Use this option to find places where "
+ + "your build may break in the future due to deprecations or other changes."
+ )
+ public Void allIncompatibleChanges;
+
@Option(name = "config",
defaultValue = "",
category = "misc",
diff --git a/src/main/java/com/google/devtools/common/options/IsolatedOptionsData.java b/src/main/java/com/google/devtools/common/options/IsolatedOptionsData.java
index 60a32a9..5074b99 100644
--- a/src/main/java/com/google/devtools/common/options/IsolatedOptionsData.java
+++ b/src/main/java/com/google/devtools/common/options/IsolatedOptionsData.java
@@ -45,7 +45,7 @@
// accepting Object values, and the List in allOptionsField should be ImmutableList. Either fix
// this or remove @Immutable.
@Immutable
-class IsolatedOptionsData extends OpaqueOptionsData {
+public class IsolatedOptionsData extends OpaqueOptionsData {
/**
* These are the options-declaring classes which are annotated with {@link Option} annotations.
@@ -181,6 +181,11 @@
return field.getType().equals(Void.class);
}
+ /** Returns whether the arg is an expansion option. */
+ public static boolean isExpansionOption(Option annotation) {
+ return (annotation.expansion().length > 0 || OptionsData.usesExpansionFunction(annotation));
+ }
+
/**
* Returns whether the arg is an expansion option defined by an expansion function (and not a
* constant expansion value).
diff --git a/src/main/java/com/google/devtools/common/options/OptionsParser.java b/src/main/java/com/google/devtools/common/options/OptionsParser.java
index 1e475e5..62f9ee7 100644
--- a/src/main/java/com/google/devtools/common/options/OptionsParser.java
+++ b/src/main/java/com/google/devtools/common/options/OptionsParser.java
@@ -66,7 +66,10 @@
* An unchecked exception thrown when there is a problem constructing a parser, e.g. an error
* while validating an {@link Option} field in one of its {@link OptionsBase} subclasses.
*
- * <p>Although unchecked, we explicitly mark some methods as throwing it as a reminder in the API.
+ * <p>This exception is unchecked because it generally indicates an internal error affecting all
+ * invocations of the program. I.e., any such error should be immediately obvious to the
+ * developer. Although unchecked, we explicitly mark some methods as throwing it as a reminder in
+ * the API.
*/
public static class ConstructionException extends RuntimeException {
public ConstructionException(String message) {
@@ -187,7 +190,7 @@
public void setAllowSingleDashLongOptions(boolean allowSingleDashLongOptions) {
this.impl.setAllowSingleDashLongOptions(allowSingleDashLongOptions);
}
-
+
/** Enables the Parser to handle params files loacted insinde the provided {@link FileSystem}. */
public void enableParamsFileSupport(FileSystem fs) {
this.impl.setArgsPreProcessor(new ParamsFilePreProcessor(fs));
@@ -273,7 +276,7 @@
return expansions;
}
}
-
+
/**
* The name and value of an option with additional metadata describing its
* priority, source, whether it was set via an implicit dependency, and if so,
@@ -450,9 +453,7 @@
}
boolean isExpansion() {
- Option option = field.getAnnotation(Option.class);
- return (option.expansion().length > 0
- || OptionsData.usesExpansionFunction(option));
+ return OptionsData.isExpansionOption(field.getAnnotation(Option.class));
}
boolean isImplicitRequirement() {
diff --git a/src/test/java/com/google/devtools/build/lib/runtime/AllIncompatibleChangesExpansionTest.java b/src/test/java/com/google/devtools/build/lib/runtime/AllIncompatibleChangesExpansionTest.java
new file mode 100644
index 0000000..c99246b
--- /dev/null
+++ b/src/test/java/com/google/devtools/build/lib/runtime/AllIncompatibleChangesExpansionTest.java
@@ -0,0 +1,342 @@
+// Copyright 2017 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.build.lib.runtime;
+
+import static com.google.common.truth.Truth.assertThat;
+import static org.junit.Assert.fail;
+
+import com.google.devtools.common.options.Converters;
+import com.google.devtools.common.options.ExpansionFunction;
+import com.google.devtools.common.options.IsolatedOptionsData;
+import com.google.devtools.common.options.Option;
+import com.google.devtools.common.options.OptionsBase;
+import com.google.devtools.common.options.OptionsParser;
+import com.google.devtools.common.options.OptionsParsingException;
+import java.util.List;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.JUnit4;
+
+/**
+ * Tests for the Incompatible Changes system (--incompatible_* flags). These go in their own suite
+ * because the options parser doesn't know the business logic for incompatible changes.
+ */
+@RunWith(JUnit4.class)
+public class AllIncompatibleChangesExpansionTest {
+
+ /** Dummy comment (linter suppression) */
+ public static class ExampleOptions extends OptionsBase {
+ @Option(
+ name = "all",
+ defaultValue = "null",
+ expansionFunction = AllIncompatibleChangesExpansion.class
+ )
+ public Void all;
+
+ @Option(name = "X", defaultValue = "false")
+ public boolean x;
+
+ @Option(name = "Y", defaultValue = "true")
+ public boolean y;
+
+ @Option(
+ name = "incompatible_A",
+ category = "incompatible changes",
+ defaultValue = "false",
+ help = "Migrate to A"
+ )
+ public boolean incompatibleA;
+
+ @Option(
+ name = "incompatible_B",
+ category = "incompatible changes",
+ defaultValue = "false",
+ help = "Migrate to B"
+ )
+ public boolean incompatibleB;
+ }
+
+ /** Dummy comment (linter suppression) */
+ public static class ExampleExpansionOptions extends OptionsBase {
+ @Option(
+ name = "incompatible_expX",
+ category = "incompatible changes",
+ defaultValue = "null",
+ expansion = {"--X"},
+ help = "Start using X"
+ )
+ public Void incompatibleExpX;
+
+ /** Dummy comment (linter suppression) */
+ public static class YExpansion implements ExpansionFunction {
+ @Override
+ public String[] getExpansion(IsolatedOptionsData optionsData) {
+ return new String[] {"--noY"};
+ }
+ }
+
+ @Option(
+ name = "incompatible_expY",
+ category = "incompatible changes",
+ defaultValue = "null",
+ expansionFunction = YExpansion.class,
+ help = "Stop using Y"
+ )
+ public Void incompatibleExpY;
+ }
+
+ @Test
+ public void noChangesSelected() throws OptionsParsingException {
+ OptionsParser parser = OptionsParser.newOptionsParser(ExampleOptions.class);
+ parser.parse("");
+ ExampleOptions opts = parser.getOptions(ExampleOptions.class);
+ assertThat(opts.x).isFalse();
+ assertThat(opts.y).isTrue();
+ assertThat(opts.incompatibleA).isFalse();
+ assertThat(opts.incompatibleB).isFalse();
+ }
+
+ @Test
+ public void allChangesSelected() throws OptionsParsingException {
+ OptionsParser parser = OptionsParser.newOptionsParser(ExampleOptions.class);
+ parser.parse("--all");
+ ExampleOptions opts = parser.getOptions(ExampleOptions.class);
+ assertThat(opts.x).isFalse();
+ assertThat(opts.y).isTrue();
+ assertThat(opts.incompatibleA).isTrue();
+ assertThat(opts.incompatibleB).isTrue();
+ }
+
+ @Test
+ public void rightmostOverrides() throws OptionsParsingException {
+ // Check that all-expansion behaves just like any other expansion flag:
+ // the rightmost setting of any individual option wins.
+ OptionsParser parser = OptionsParser.newOptionsParser(ExampleOptions.class);
+ parser.parse("--noincompatible_A", "--all", "--noincompatible_B");
+ ExampleOptions opts = parser.getOptions(ExampleOptions.class);
+ assertThat(opts.incompatibleA).isTrue();
+ assertThat(opts.incompatibleB).isFalse();
+ }
+
+ @Test
+ public void expansionOptions() throws OptionsParsingException {
+ // Check that all-expansion behaves just like any other expansion flag:
+ // the rightmost setting of any individual option wins.
+ OptionsParser parser =
+ OptionsParser.newOptionsParser(ExampleOptions.class, ExampleExpansionOptions.class);
+ parser.parse("--all");
+ ExampleOptions opts = parser.getOptions(ExampleOptions.class);
+ assertThat(opts.x).isTrue();
+ assertThat(opts.y).isFalse();
+ assertThat(opts.incompatibleA).isTrue();
+ assertThat(opts.incompatibleB).isTrue();
+ }
+
+ // There's no unit test to check that the expansion of --all is sorted. IsolatedOptionsData is not
+ // exposed from OptionsParser, making it difficult to check, and it's not clear that exposing it
+ // would be worth it.
+
+ /**
+ * Ensure that we get an {@link OptionsParser.ConstructionException} containing {@code message}
+ * when the incompatible changes in the given {@link OptionsBase} subclass are validated.
+ */
+ // Because javadoc can't resolve inner classes.
+ @SuppressWarnings("javadoc")
+ private static void assertBadness(Class<? extends OptionsBase> optionsBaseClass, String message) {
+ try {
+ OptionsParser.newOptionsParser(ExampleOptions.class, optionsBaseClass);
+ fail("Should have failed with message \"" + message + "\"");
+ } catch (OptionsParser.ConstructionException e) {
+ assertThat(e.getMessage()).contains(message);
+ }
+ }
+
+ /** Dummy comment (linter suppression) */
+ public static class BadNameOptions extends OptionsBase {
+ @Option(
+ name = "badname",
+ category = "incompatible changes",
+ defaultValue = "false",
+ help = "nohelp"
+ )
+ public boolean bad;
+ }
+
+ @Test
+ public void badName() {
+ assertBadness(
+ BadNameOptions.class,
+ "Incompatible change option '--badname' must have name "
+ + "starting with \"incompatible_\"");
+ }
+
+ /** Dummy comment (linter suppression) */
+ public static class BadCategoryOptions extends OptionsBase {
+ @Option(name = "incompatible_bad", category = "badcat", defaultValue = "false", help = "nohelp")
+ public boolean bad;
+ }
+
+ @Test
+ public void badCategory() {
+ assertBadness(BadCategoryOptions.class, "must have category \"incompatible changes\"");
+ }
+
+ /** Dummy comment (linter suppression) */
+ public static class BadTypeOptions extends OptionsBase {
+ @Option(
+ name = "incompatible_bad",
+ category = "incompatible changes",
+ defaultValue = "0",
+ help = "nohelp"
+ )
+ public int bad;
+ }
+
+ @Test
+ public void badType() {
+ assertBadness(BadTypeOptions.class, "must have boolean type");
+ }
+
+ /** Dummy comment (linter suppression) */
+ public static class BadHelpOptions extends OptionsBase {
+ @Option(name = "incompatible_bad", category = "incompatible changes", defaultValue = "false")
+ public boolean bad;
+ }
+
+ @Test
+ public void badHelp() {
+ assertBadness(BadHelpOptions.class, "must have a \"help\" string");
+ }
+
+ /** Dummy comment (linter suppression) */
+ public static class BadAbbrevOptions extends OptionsBase {
+ @Option(
+ name = "incompatible_bad",
+ category = "incompatible changes",
+ defaultValue = "false",
+ help = "nohelp",
+ abbrev = 'x'
+ )
+ public boolean bad;
+ }
+
+ @Test
+ public void badAbbrev() {
+ assertBadness(BadAbbrevOptions.class, "must not use the abbrev field");
+ }
+
+ /** Dummy comment (linter suppression) */
+ public static class BadValueHelpOptions extends OptionsBase {
+ @Option(
+ name = "incompatible_bad",
+ category = "incompatible changes",
+ defaultValue = "false",
+ help = "nohelp",
+ valueHelp = "x"
+ )
+ public boolean bad;
+ }
+
+ @Test
+ public void badValueHelp() {
+ assertBadness(BadValueHelpOptions.class, "must not use the valueHelp field");
+ }
+
+ /** Dummy comment (linter suppression) */
+ public static class BadConverterOptions extends OptionsBase {
+ @Option(
+ name = "incompatible_bad",
+ category = "incompatible changes",
+ defaultValue = "false",
+ help = "nohelp",
+ converter = Converters.BooleanConverter.class
+ )
+ public boolean bad;
+ }
+
+ @Test
+ public void badConverter() {
+ assertBadness(BadConverterOptions.class, "must not use the converter field");
+ }
+
+ /** Dummy comment (linter suppression) */
+ public static class BadAllowMultipleOptions extends OptionsBase {
+ @Option(
+ name = "incompatible_bad",
+ category = "incompatible changes",
+ defaultValue = "null",
+ help = "nohelp",
+ allowMultiple = true
+ )
+ public List<String> bad;
+ }
+
+ @Test
+ public void badAllowMutliple() {
+ assertBadness(BadAllowMultipleOptions.class, "must not use the allowMultiple field");
+ }
+
+ /** Dummy comment (linter suppression) */
+ public static class BadImplicitRequirementsOptions extends OptionsBase {
+ @Option(
+ name = "incompatible_bad",
+ category = "incompatible changes",
+ defaultValue = "false",
+ help = "nohelp",
+ implicitRequirements = "--x"
+ )
+ public boolean bad;
+ }
+
+ @Test
+ public void badImplicitRequirements() {
+ assertBadness(
+ BadImplicitRequirementsOptions.class, "must not use the implicitRequirements field");
+ }
+
+ /** Dummy comment (linter suppression) */
+ public static class BadOldNameOptions extends OptionsBase {
+ @Option(
+ name = "incompatible_bad",
+ category = "incompatible changes",
+ defaultValue = "false",
+ help = "nohelp",
+ oldName = "x"
+ )
+ public boolean bad;
+ }
+
+ @Test
+ public void badOldName() {
+ assertBadness(BadOldNameOptions.class, "must not use the oldName field");
+ }
+
+ /** Dummy comment (linter suppression) */
+ public static class BadWrapperOptionOptions extends OptionsBase {
+ @Option(
+ name = "incompatible_bad",
+ category = "incompatible changes",
+ defaultValue = "false",
+ help = "nohelp",
+ wrapperOption = true
+ )
+ public boolean bad;
+ }
+
+ @Test
+ public void badWrapperOption() {
+ assertBadness(BadWrapperOptionOptions.class, "must not use the wrapperOption field");
+ }
+}
