diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/CoreOptions.java b/src/main/java/com/google/devtools/build/lib/analysis/config/CoreOptions.java index 5a9090644f9352..7bcd6d75fdd4d0 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/config/CoreOptions.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/config/CoreOptions.java @@ -34,9 +34,12 @@ import com.google.devtools.common.options.OptionsParsingException; import com.google.devtools.common.options.TriState; import java.util.AbstractMap.SimpleEntry; +import java.util.ArrayList; +import java.util.HashMap; import java.util.LinkedHashMap; import java.util.List; import java.util.Map; +import java.util.TreeSet; /** * Core options affecting a {@link BuildConfigurationValue} that don't belong in domain-specific @@ -1016,6 +1019,40 @@ public LinkedHashMap getNormalizedCommandLineBuildVariables() { return flagValueByName; } + /// Normalizes --features flags by sorting the values and having disables win over enables. + private static List getNormalizedFeatures(List features) { + // Parse out the features into a Map, where the boolean represents whether + // the feature is enabled or disabled. + Map featureToState = new HashMap<>(); + for (String feature : features) { + if (feature.startsWith("-")) { + // disable always wins. + featureToState.put(feature.substring(1), false); + } else if (!featureToState.containsKey(feature)) { + // enable feature only if it does not already have a state. + // If existing state is enabled, no need to do extra work. + // If existing state is disabled, it wins. + featureToState.put(feature, true); + } + } + // Partition into enabled/disabled features. + TreeSet enabled = new TreeSet<>(); + TreeSet disabled = new TreeSet<>(); + for (Map.Entry entry : featureToState.entrySet()) { + if (entry.getValue()) { + enabled.add(entry.getKey()); + } else { + disabled.add(entry.getKey()); + } + } + // Rebuild the set of features. + // Since we used TreeSet the features come out in a deterministic order. + List result = new ArrayList<>(enabled); + disabled.stream().map(x -> "-" + x).forEach(result::add); + // If we made no changes, return the same instance we got to reduce churn. + return result.equals(features) ? features : result; + } + @Override public CoreOptions getNormalized() { CoreOptions result = (CoreOptions) clone(); @@ -1033,6 +1070,8 @@ public CoreOptions getNormalized() { .collect(toImmutableList()); } } + // Normalize features. + result.defaultFeatures = getNormalizedFeatures(defaultFeatures); return result; } diff --git a/src/test/java/com/google/devtools/build/lib/analysis/config/CoreOptionsTest.java b/src/test/java/com/google/devtools/build/lib/analysis/config/CoreOptionsTest.java new file mode 100644 index 00000000000000..092d75c71f6118 --- /dev/null +++ b/src/test/java/com/google/devtools/build/lib/analysis/config/CoreOptionsTest.java @@ -0,0 +1,82 @@ +// Copyright 2023 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.analysis.config; + +import com.google.common.collect.ImmutableList; +import com.google.devtools.build.lib.analysis.util.OptionsTestCase; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +/** A test for {@link CoreOptions}. */ +@RunWith(JUnit4.class) +public final class CoreOptionsTest extends OptionsTestCase { + + private static final String FEATURES_PREFIX = "--features="; + private static final String DEFINE_PREFIX = "--define="; + + private static final ImmutableList NO_COLLAPSE_DEFINES = + ImmutableList.of("--nocollapse_duplicate_defines"); + private static final ImmutableList COLLAPSE_DEFINES = + ImmutableList.of("--collapse_duplicate_defines"); + + @Test + public void testFeatures_orderingOfPositiveFeatures() throws Exception { + CoreOptions one = createWithPrefix(FEATURES_PREFIX, "foo", "bar"); + CoreOptions two = createWithPrefix(FEATURES_PREFIX, "bar", "foo"); + assertSame(one, two); + } + + @Test + public void testFeatures_duplicateFeatures() throws Exception { + CoreOptions one = createWithPrefix(FEATURES_PREFIX, "foo", "bar"); + CoreOptions two = createWithPrefix(FEATURES_PREFIX, "bar", "foo", "bar"); + assertSame(one, two); + } + + @Test + public void testFeatures_disablingWins() throws Exception { + CoreOptions one = createWithPrefix(FEATURES_PREFIX, "foo", "-foo", "bar"); + CoreOptions two = createWithPrefix(FEATURES_PREFIX, "-foo", "bar"); + assertSame(one, two); + } + + @Test + public void testDefines_ordering_effectOnCacheKey() throws Exception { + CoreOptions one = createWithPrefix(NO_COLLAPSE_DEFINES, DEFINE_PREFIX, "a=1", "b=2"); + CoreOptions two = createWithPrefix(NO_COLLAPSE_DEFINES, DEFINE_PREFIX, "b=2", "a=1"); + // Due to NO_COLLAPSE_DEFINES, the two configurations are not equivalent. + assertDifferent(one, two); + } + + @Test + public void testDefines_duplicates_effectOnCacheKey() throws Exception { + CoreOptions one = createWithPrefix(NO_COLLAPSE_DEFINES, DEFINE_PREFIX, "a=2", "a=2"); + CoreOptions two = createWithPrefix(NO_COLLAPSE_DEFINES, DEFINE_PREFIX, "a=2"); + // Due to NO_COLLAPSE_DEFINES, the two configurations are not equivalent. + assertDifferent(one, two); + } + + @Test + public void testDefines_duplicatesWithCollapsingEnabled_effectOnCacheKey() throws Exception { + CoreOptions one = createWithPrefix(COLLAPSE_DEFINES, DEFINE_PREFIX, "a=1", "a=2"); + CoreOptions two = createWithPrefix(COLLAPSE_DEFINES, DEFINE_PREFIX, "a=2"); + assertSame(one, two); + } + + @Override + protected Class getOptionsClass() { + return CoreOptions.class; + } +} diff --git a/src/test/java/com/google/devtools/build/lib/analysis/util/OptionsTestCase.java b/src/test/java/com/google/devtools/build/lib/analysis/util/OptionsTestCase.java new file mode 100644 index 00000000000000..fae119d8663c23 --- /dev/null +++ b/src/test/java/com/google/devtools/build/lib/analysis/util/OptionsTestCase.java @@ -0,0 +1,84 @@ +// Copyright 2023 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.analysis.util; + +import static com.google.common.truth.Truth.assertThat; +import static java.util.Arrays.stream; + +import com.google.common.collect.ImmutableList; +import com.google.devtools.build.lib.analysis.config.FragmentOptions; +import com.google.devtools.common.options.OptionsParser; +import java.util.Arrays; +import java.util.List; + +/** A base class for testing cacheKey related functionality of Option classes. */ +public abstract class OptionsTestCase { + + protected abstract Class getOptionsClass(); + + /** Construct options parsing the given arguments. */ + protected T create(List args) throws Exception { + Class cls = getOptionsClass(); + OptionsParser parser = OptionsParser.builder().optionsClasses(ImmutableList.of(cls)).build(); + parser.parse(args); + return parser.getOptions(cls); + } + + /** Construct options parsing the given arguments. */ + protected T create(String... args) throws Exception { + return create(Arrays.asList(args)); + } + + /** + * Useful for options which are specified multiple times on the command line. {@code + * createWithPrefix("--abc=", "x", "y", "z")} is equivalent to {@code create("--abc=x", "--abc=y", + * "--abc=z")} + */ + protected T createWithPrefix(String prefix, String... args) throws Exception { + return createWithPrefix(ImmutableList.of(), prefix, args); + } + + /** + * Variant of {@link #createWithPrefix(String, String...)} with additional fixed set of options. + */ + protected T createWithPrefix(ImmutableList fixed, String prefix, String... args) + throws Exception { + ImmutableList.Builder builder = ImmutableList.builder(); + builder.addAll(fixed); + stream(args).map(x -> prefix + x).forEach(builder::add); + return create(builder.build()); + } + + protected void assertSame(T one, T two) { + // We normalize first, since that is what BuildOptions.checkSum() does. + // We do not use BuildOptions.checkSum() because in case of test failure, + // the diff on cacheKey is humanreadable. + FragmentOptions oneNormalized = one.getNormalized(); + FragmentOptions twoNormalized = two.getNormalized(); + assertThat(oneNormalized.cacheKey()).isEqualTo(twoNormalized.cacheKey()); + // Also check equality of toString() as that influences the ST-hash computation. + assertThat(oneNormalized.toString()).isEqualTo(twoNormalized.toString()); + } + + protected void assertDifferent(T one, T two) { + // We normalize first, since that is what BuildOptions.checkSum() does. + // We do not use BuildOptions.checkSum() because in case of test failure, + // the diff on cacheKey is humanreadable. + FragmentOptions oneNormalized = one.getNormalized(); + FragmentOptions twoNormalized = two.getNormalized(); + assertThat(oneNormalized.cacheKey()).isNotEqualTo(twoNormalized.cacheKey()); + // Also check equality of toString() as that influences the ST-hash computation. + assertThat(oneNormalized.toString()).isNotEqualTo(twoNormalized.toString()); + } +}