Fix crash on poorly formed config_setting --defines.
Specifically:
config_setting(
name = "config",
values = {"define": "abc"})
This breaks the invariant that all --define values must be of "a=b" form.
With this fix, we revert back to standard behavior of a clearly reported error.
PiperOrigin-RevId: 295222617
diff --git a/src/main/java/com/google/devtools/build/lib/rules/config/ConfigSetting.java b/src/main/java/com/google/devtools/build/lib/rules/config/ConfigSetting.java
index 63052ca..ae70e54 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/config/ConfigSetting.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/config/ConfigSetting.java
@@ -290,22 +290,24 @@
continue;
}
- requiredFragmentOptions.add(
- optionName.equals("define")
- // --define is more like user-defined build flags than traditional native flags.
- // Report it
- // like user-defined flags: the dependency is directly on the flag vs. the fragment
- // that
- // contains the flag. This frees a rule that depends on "--define a=1" from preserving
- // another rule's dependency on "--define b=2". In other words, if both rules simply
- // said
- // "I require CoreOptions" (which is the FragmentOptions --define belongs to), that
- // would
- // hide the reality that they really have orthogonal dependencies: removing
- // "--define b=2" is perfectly safe for the rule that needs "--define a=1".
- ? "--define:" + expectedRawValue.substring(0, expectedRawValue.indexOf('='))
- // For other native flags, it's reasonable to report the fragment they belong to.
- : ClassName.getSimpleNameWithOuter(optionClass));
+ if (optionName.equals("define")) {
+ // --define is more like user-defined build flags than traditional native flags. Report it
+ // like user-defined flags: the dependency is directly on the flag vs. the fragment that
+ // contains the flag. This frees a rule that depends on "--define a=1" from preserving
+ // another rule's dependency on "--define b=2". In other words, if both rules simply said
+ // "I require CoreOptions" (which is the FragmentOptions --define belongs to), that would
+ // hide the reality that they really have orthogonal dependencies: removing
+ // "--define b=2" is perfectly safe for the rule that needs "--define a=1".
+ int equalsIndex = expectedRawValue.indexOf('=');
+ requiredFragmentOptions.add(
+ "--define:"
+ + (equalsIndex > 0
+ ? expectedRawValue.substring(0, equalsIndex)
+ : expectedRawValue));
+ } else {
+ // For other native flags, it's reasonable to report the fragment they belong to.
+ requiredFragmentOptions.add(ClassName.getSimpleNameWithOuter(optionClass));
+ }
SelectRestriction selectRestriction = options.getSelectRestriction(optionName);
if (selectRestriction != null) {
diff --git a/src/test/java/com/google/devtools/build/lib/rules/config/ConfigSettingTest.java b/src/test/java/com/google/devtools/build/lib/rules/config/ConfigSettingTest.java
index 323ddea..4e3f7d4 100644
--- a/src/test/java/com/google/devtools/build/lib/rules/config/ConfigSettingTest.java
+++ b/src/test/java/com/google/devtools/build/lib/rules/config/ConfigSettingTest.java
@@ -391,6 +391,20 @@
}
@Test
+ public void invalidDefineProducesError() throws Exception {
+ scratch.file(
+ "test/BUILD",
+ "config_setting(",
+ " name = 'match',",
+ " values = {",
+ " 'define': 'foo',", // Value should be "foo=<something>".
+ " })");
+
+ checkError(
+ "//test:match", "Variable definitions must be in the form of a 'name=value' assignment");
+ }
+
+ @Test
public void multipleDefines() throws Exception {
scratch.file("test/BUILD",
"config_setting(",
