From 1e4b9977ccc07b6a0cbc75c5a16380fdde95efe7 Mon Sep 17 00:00:00 2001
From: gregce <gregce@google.com>
Date: Thu, 30 Jan 2020 08:12:06 -0800
Subject: [PATCH] cquery --show_config_fragments: report --define requirements
 more precisely.

Before this change, any reliance on --define a=foo reports a dependency on the
"CoreOptions" fragment. While true, that's too broad: it includes *all*
--defines, including ones having nothing to do with "a".

This change reports --define like user-defined flags (which it essentially
is a special-case version of): return the specific --define instead. In the
above example that'd be --define:a (using a syntax that's easy enough to read
while being reasonably parseable as a no-spaces token).

This only works for select(). It doesn't include MAKE variables, e.g.:
https://github.com/bazelbuild/bazel/blob/6bd6cc4435e722393127c72189dbb646149d844b/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java#L610-L621. That
requires its own
analysis.

Prereq work for https://github.com/bazelbuild/bazel/issues/10613.

PiperOrigin-RevId: 292350812
---
 .../build/lib/rules/config/ConfigSetting.java | 18 ++++++++++++-
 .../integration/configured_query_test.sh      | 26 +++++++++++++++++++
 2 files changed, 43 insertions(+), 1 deletion(-)

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 4b4ef3bb277d29..63052ca66fef60 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
@@ -289,7 +289,23 @@ private static boolean matchesConfig(
         foundMismatch = true;
         continue;
       }
-      requiredFragmentOptions.add(ClassName.getSimpleNameWithOuter(optionClass));
+
+      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));
 
       SelectRestriction selectRestriction = options.getSelectRestriction(optionName);
       if (selectRestriction != null) {
diff --git a/src/test/shell/integration/configured_query_test.sh b/src/test/shell/integration/configured_query_test.sh
index da3294a7c2d284..f52143b3d696a8 100755
--- a/src/test/shell/integration/configured_query_test.sh
+++ b/src/test/shell/integration/configured_query_test.sh
@@ -592,6 +592,32 @@ EOF
   assert_not_contains "//$pkg:foo_feature .*//$pkg:foo_feature" output
 }
 
+function test_show_config_fragments_on_define() {
+  local -r pkg=$FUNCNAME
+  mkdir -p $pkg
+  cat > $pkg/BUILD <<EOF
+config_setting(
+    name = "is_a_on",
+    define_values = {"a": "on"}
+)
+
+cc_library(
+    name = "cclib_with_select",
+    srcs = select({
+        ":is_a_on": ["version1.cc"],
+        "//conditions:default": ["version2.cc"],
+    })
+)
+EOF
+
+  bazel cquery "//$pkg:all" --show_config_fragments=direct --define a=on \
+    --define b=on > output 2>"$TEST_log" || fail "Expected success"
+
+  assert_contains "//$pkg:cclib_with_select .*CppConfiguration" output
+  assert_contains "//$pkg:cclib_with_select .*--define:a" output
+  assert_not_contains "//$pkg:cclib_with_select .*--define:b" output
+}
+
 function test_manual_tagged_targets_always_included_for_queries() {
   local -r pkg=$FUNCNAME
   mkdir -p $pkg