Expose sysroot as a build variable

This cl removes hardcoded --sysroot flag generation from bazel when constructing
command line for C++ actions. The hardcoded flag is still exposed to Skylark (to
stay backwards compatible).

RELNOTES: None.
PiperOrigin-RevId: 168346711

8 files changed, 120 insertions, 84 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainProvider.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainProvider.java
index 6526877565..8f85034126 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainProvider.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainProvider.java
@@ -355,11 +355,11 @@ public final class CcToolchainProvider extends ToolchainInfo {
             + "rules. These should be appended to the command line after filtering."
   )
   public ImmutableList<String> getUnfilteredCompilerOptionsWithSysroot(Iterable<String> features) {
-    return cppConfiguration.getUnfilteredCompilerOptions(features, sysroot);
+    return cppConfiguration.getUnfilteredCompilerOptionsDoNotUse(features, sysroot);
   }
 
   public ImmutableList<String> getUnfilteredCompilerOptions(Iterable<String> features) {
-    return cppConfiguration.getUnfilteredCompilerOptions(features, null);
+    return cppConfiguration.getUnfilteredCompilerOptionsDoNotUse(features, /* sysroot= */ null);
   }
 
   @SkylarkCallable(
@@ -369,10 +369,15 @@ public final class CcToolchainProvider extends ToolchainInfo {
         "Returns the set of command-line linker options, including any flags "
             + "inferred from the command-line options."
   )
+  public ImmutableList<String> getLinkOptionsWithSysroot() {
+    return cppConfiguration.getLinkOptionsDoNotUse(sysroot);
+  }
+
   public ImmutableList<String> getLinkOptions() {
-    return cppConfiguration.getLinkOptions(sysroot);
+    return cppConfiguration.getLinkOptionsDoNotUse(/* sysroot= */ null);
   }
 
+
   // Not all of CcToolchainProvider is exposed to Skylark, which makes implementing deep equality
   // impossible: if Java-only parts are considered, the behavior is surprising in Skylark, if they
   // are not, the behavior is surprising in Java. Thus, object identity it is.
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CompileCommandLine.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CompileCommandLine.java
index 33d282e2b4..3215cbb36e 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CompileCommandLine.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CompileCommandLine.java
@@ -39,7 +39,6 @@ public final class CompileCommandLine {
   private final CcToolchainFeatures.Variables variables;
   private final String actionName;
   private final CppConfiguration cppConfiguration;
-  private final CcToolchainProvider cppProvider;
   private final DotdFile dotdFile;
 
   private CompileCommandLine(
@@ -50,8 +49,7 @@ public final class CompileCommandLine {
       CppConfiguration cppConfiguration,
       CcToolchainFeatures.Variables variables,
       String actionName,
-      DotdFile dotdFile,
-      CcToolchainProvider cppProvider) {
+      DotdFile dotdFile) {
     this.sourceFile = Preconditions.checkNotNull(sourceFile);
     this.outputFile = Preconditions.checkNotNull(outputFile);
     this.coptsFilter = coptsFilter;
@@ -60,7 +58,6 @@ public final class CompileCommandLine {
     this.variables = variables;
     this.actionName = actionName;
     this.dotdFile = isGenerateDotdFile(sourceFile) ? Preconditions.checkNotNull(dotdFile) : null;
-    this.cppProvider = cppProvider;
   }
 
   /** Returns true if Dotd file should be generated. */
@@ -119,13 +116,6 @@ public final class CompileCommandLine {
     addFilteredOptions(
         options, featureConfiguration.getPerFeatureExpansions(actionName, updatedVariables));
 
-    if (isObjcCompile(actionName)) {
-      PathFragment sysroot = cppProvider.getSysroot();
-      if (sysroot != null) {
-        options.add(cppConfiguration.getSysrootCompilerOption(sysroot));
-      }
-    }
-
     if (!featureConfiguration.isEnabled("compile_action_flags_in_flag_set")) {
       if (FileType.contains(outputFile, CppFileTypes.ASSEMBLER, CppFileTypes.PIC_ASSEMBLER)) {
         options.add("-S");
@@ -142,11 +132,6 @@ public final class CompileCommandLine {
     return options;
   }
 
-  private boolean isObjcCompile(String actionName) {
-    return (actionName.equals(CppCompileAction.OBJC_COMPILE)
-        || actionName.equals(CppCompileAction.OBJCPP_COMPILE));
-  }
-
   // For each option in 'in', add it to 'out' unless it is matched by the 'coptsFilter' regexp.
   private void addFilteredOptions(
       List<String> out, List<Pair<String, List<String>>> expandedFeatures) {
@@ -193,16 +178,9 @@ public final class CompileCommandLine {
       Predicate<String> coptsFilter,
       String actionName,
       CppConfiguration cppConfiguration,
-      DotdFile dotdFile,
-      CcToolchainProvider cppProvider) {
+      DotdFile dotdFile) {
     return new Builder(
-        sourceFile,
-        outputFile,
-        coptsFilter,
-        actionName,
-        cppConfiguration,
-        dotdFile,
-        cppProvider);
+        sourceFile, outputFile, coptsFilter, actionName, cppConfiguration, dotdFile);
   }
 
   /** A builder for a {@link CompileCommandLine}. */
@@ -215,7 +193,6 @@ public final class CompileCommandLine {
     private final String actionName;
     private final CppConfiguration cppConfiguration;
     @Nullable private final DotdFile dotdFile;
-    private final CcToolchainProvider ccToolchainProvider;
 
     public CompileCommandLine build() {
       return new CompileCommandLine(
@@ -226,8 +203,7 @@ public final class CompileCommandLine {
           Preconditions.checkNotNull(cppConfiguration),
           Preconditions.checkNotNull(variables),
           Preconditions.checkNotNull(actionName),
-          dotdFile,
-          Preconditions.checkNotNull(ccToolchainProvider));
+          dotdFile);
     }
 
     private Builder(
@@ -236,15 +212,13 @@ public final class CompileCommandLine {
         Predicate<String> coptsFilter,
         String actionName,
         CppConfiguration cppConfiguration,
-        DotdFile dotdFile,
-        CcToolchainProvider ccToolchainProvider) {
+        DotdFile dotdFile) {
       this.sourceFile = sourceFile;
       this.outputFile = outputFile;
       this.coptsFilter = coptsFilter;
       this.actionName = actionName;
       this.cppConfiguration = cppConfiguration;
       this.dotdFile = dotdFile;
-      this.ccToolchainProvider = ccToolchainProvider;
     }
 
     /** Sets the feature configuration for this compile action. */
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppActionConfigs.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppActionConfigs.java
index 958b26f7f6..e965d4a82f 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppActionConfigs.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppActionConfigs.java
@@ -68,6 +68,7 @@ public class CppActionConfigs {
                 "  }",
                 "  implies: 'legacy_compile_flags'",
                 "  implies: 'user_compile_flags'",
+                "  implies: 'sysroot'",
                 "  implies: 'unfiltered_compile_flags'",
                 "}",
                 "action_config {",
@@ -78,6 +79,7 @@ public class CppActionConfigs {
                 "  }",
                 "  implies: 'legacy_compile_flags'",
                 "  implies: 'user_compile_flags'",
+                "  implies: 'sysroot'",
                 "  implies: 'unfiltered_compile_flags'",
                 "}",
                 "action_config {",
@@ -88,6 +90,7 @@ public class CppActionConfigs {
                 "  }",
                 "  implies: 'legacy_compile_flags'",
                 "  implies: 'user_compile_flags'",
+                "  implies: 'sysroot'",
                 "  implies: 'unfiltered_compile_flags'",
                 "}",
                 "action_config {",
@@ -98,6 +101,7 @@ public class CppActionConfigs {
                 "  }",
                 "  implies: 'legacy_compile_flags'",
                 "  implies: 'user_compile_flags'",
+                "  implies: 'sysroot'",
                 "  implies: 'unfiltered_compile_flags'",
                 "}",
                 "action_config {",
@@ -108,6 +112,7 @@ public class CppActionConfigs {
                 "  }",
                 "  implies: 'legacy_compile_flags'",
                 "  implies: 'user_compile_flags'",
+                "  implies: 'sysroot'",
                 "  implies: 'unfiltered_compile_flags'",
                 "}",
                 "action_config {",
@@ -118,6 +123,7 @@ public class CppActionConfigs {
                 "  }",
                 "  implies: 'legacy_compile_flags'",
                 "  implies: 'user_compile_flags'",
+                "  implies: 'sysroot'",
                 "  implies: 'unfiltered_compile_flags'",
                 "}",
                 "action_config {",
@@ -128,6 +134,7 @@ public class CppActionConfigs {
                 "  }",
                 "  implies: 'legacy_compile_flags'",
                 "  implies: 'user_compile_flags'",
+                "  implies: 'sysroot'",
                 "  implies: 'unfiltered_compile_flags'",
                 "}",
                 "action_config {",
@@ -138,6 +145,7 @@ public class CppActionConfigs {
                 "  }",
                 "  implies: 'legacy_compile_flags'",
                 "  implies: 'user_compile_flags'",
+                "  implies: 'sysroot'",
                 "  implies: 'unfiltered_compile_flags'",
                 "}",
                 ifTrue(
@@ -376,6 +384,7 @@ public class CppActionConfigs {
                 "  implies: 'legacy_link_flags'",
                 "  implies: 'linker_param_file'",
                 "  implies: 'fission_support'",
+                "  implies: 'sysroot'",
                 "}",
                 "action_config {",
                 "  config_name: 'c++-link-dynamic-library'",
@@ -398,6 +407,7 @@ public class CppActionConfigs {
                 "  implies: 'legacy_link_flags'",
                 "  implies: 'linker_param_file'",
                 "  implies: 'fission_support'",
+                "  implies: 'sysroot'",
                 "}",
                 "action_config {",
                 "  config_name: 'c++-link-static-library'",
@@ -813,29 +823,6 @@ public class CppActionConfigs {
                     "  }",
                     "}"),
                 ifTrue(
-                    !existingFeatureNames.contains("linker_param_file"),
-                    "feature {",
-                    "  name: 'linker_param_file'",
-                    "  flag_set {",
-                    "    expand_if_all_available: 'linker_param_file'",
-                    "    action: 'c++-link-executable'",
-                    "    action: 'c++-link-dynamic-library'",
-                    "    flag_group {",
-                    "      flag: '-Wl,@%{linker_param_file}'",
-                    "    }",
-                    "  }",
-                    "  flag_set {",
-                    "    expand_if_all_available: 'linker_param_file'",
-                    "    action: 'c++-link-static-library'",
-                    "    action: 'c++-link-alwayslink-static-library'",
-                    "    action: 'c++-link-pic-static-library'",
-                    "    action: 'c++-link-alwayslink-pic-static-library'",
-                    "    flag_group {",
-                    "      flag: '@%{linker_param_file}'",
-                    "    }",
-                    "  }",
-                    "}"),
-                ifTrue(
                     !existingFeatureNames.contains(CppRuleClasses.COVERAGE),
                     "feature {",
                     "  name: 'coverage'",
@@ -969,6 +956,28 @@ public class CppActionConfigs {
                     "    }",
                     "  }",
                     "}"),
+                ifTrue(
+                    !existingFeatureNames.contains("sysroot"),
+                    "feature {",
+                    "  name: 'sysroot'",
+                    "  enabled: true",
+                    "  flag_set {",
+                    "    action: 'preprocess-assemble'",
+                    "    action: 'c-compile'",
+                    "    action: 'c++-compile'",
+                    "    action: 'c++-header-parsing'",
+                    "    action: 'c++-header-preprocessing'",
+                    "    action: 'c++-module-compile'",
+                    "    action: 'c++-link-executable'",
+                    "    action: 'c++-link-dynamic-library'",
+                    "    action: 'clif-match'",
+                    "    action: 'lto-backend'",
+                    "    flag_group {",
+                    "      expand_if_all_available: 'sysroot'",
+                    "      flag: '--sysroot=%{sysroot}'",
+                    "    }",
+                    "  }",
+                    "}"),
                 // unfiltered_compile_flags contain system include paths. These must be added
                 // after the user provided options (present in legacy_compile_flags build
                 // variable above), otherwise users adding include paths will not pick up their own
@@ -995,6 +1004,29 @@ public class CppActionConfigs {
                     "      flag: '%{unfiltered_compile_flags}'",
                     "    }",
                     "  }",
+                    "}"),
+                ifTrue(
+                    !existingFeatureNames.contains("linker_param_file"),
+                    "feature {",
+                    "  name: 'linker_param_file'",
+                    "  flag_set {",
+                    "    expand_if_all_available: 'linker_param_file'",
+                    "    action: 'c++-link-executable'",
+                    "    action: 'c++-link-dynamic-library'",
+                    "    flag_group {",
+                    "      flag: '-Wl,@%{linker_param_file}'",
+                    "    }",
+                    "  }",
+                    "  flag_set {",
+                    "    expand_if_all_available: 'linker_param_file'",
+                    "    action: 'c++-link-static-library'",
+                    "    action: 'c++-link-alwayslink-static-library'",
+                    "    action: 'c++-link-pic-static-library'",
+                    "    action: 'c++-link-alwayslink-pic-static-library'",
+                    "    flag_group {",
+                    "      flag: '@%{linker_param_file}'",
+                    "    }",
+                    "  }",
                     "}")));
   }
 
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java
index 84a58026ad..0de697be74 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java
@@ -345,8 +345,7 @@ public class CppCompileAction extends AbstractAction
                 coptsFilter,
                 actionName,
                 cppConfiguration,
-                dotdFile,
-                cppProvider)
+                dotdFile)
             .setFeatureConfiguration(featureConfiguration)
             .setVariables(variables)
             .build();
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfiguration.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfiguration.java
index 539883bb72..4133decc06 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfiguration.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfiguration.java
@@ -1139,31 +1139,40 @@ public class CppConfiguration extends BuildConfiguration.Fragment {
   /**
    * Returns the default list of options which cannot be filtered by BUILD rules. These should be
    * appended to the command line after filtering.
+   *
+   * @deprecated since it uses nonconfigured sysroot. Use
+   * {@link CcToolchainProvider#getUnfilteredCompilerOptionsWithSysroot(Iterable)} if you *really*
+   * need to.
    */
+  // TODO(b/65401585): Migrate existing uses to cc_toolchain and cleanup here.
+  @Deprecated
   @SkylarkCallable(
     name = "unfiltered_compiler_options",
     doc =
         "Returns the default list of options which cannot be filtered by BUILD "
             + "rules. These should be appended to the command line after filtering."
   )
-  public ImmutableList<String> getUnfilteredCompilerOptions(Iterable<String> features) {
-    return getUnfilteredCompilerOptions(features, nonConfiguredSysroot);
+  public ImmutableList<String> getUnfilteredCompilerOptionsWithLegacySysroot(
+      Iterable<String> features) {
+    return getUnfilteredCompilerOptionsDoNotUse(features, nonConfiguredSysroot);
   }
 
-  public ImmutableList<String> getUnfilteredCompilerOptions(
-      Iterable<String> features, PathFragment sysroot) {
+  /**
+   * @deprecated since it hardcodes --sysroot flag. Use
+   * {@link com.google.devtools.build.lib.rules.cpp.CcToolchainFeatures.FeatureConfiguration}
+   * instead.
+   */
+  // TODO(b/65401585): Migrate existing uses to cc_toolchain and cleanup here.
+  @Deprecated
+  ImmutableList<String> getUnfilteredCompilerOptionsDoNotUse(
+      Iterable<String> features, @Nullable PathFragment sysroot) {
     if (sysroot == null) {
       return unfilteredCompilerFlags.evaluate(features);
-    } else {
-      return ImmutableList.<String>builder()
-          .add(getSysrootCompilerOption(sysroot))
-          .addAll(unfilteredCompilerFlags.evaluate(features))
-          .build();
     }
-  }
-
-  public String getSysrootCompilerOption(PathFragment sysroot) {
-    return "--sysroot=" + sysroot;
+    return ImmutableList.<String>builder()
+        .add("--sysroot=" + sysroot)
+        .addAll(unfilteredCompilerFlags.evaluate(features))
+        .build();
   }
 
   /**
@@ -1171,8 +1180,11 @@ public class CppConfiguration extends BuildConfiguration.Fragment {
    * command-line options.
    *
    * @see Link
+   * @deprecated since it uses nonconfigured sysroot. Use
+   * {@link CcToolchainProvider#getLinkOptionsWithSysroot()} if you *really* need to.
    */
-  // TODO(bazel-team): Clean up the linker options computation!
+  // TODO(b/65401585): Migrate existing uses to cc_toolchain and cleanup here.
+  @Deprecated
   @SkylarkCallable(
     name = "link_options",
     structField = true,
@@ -1180,11 +1192,18 @@ public class CppConfiguration extends BuildConfiguration.Fragment {
         "Returns the set of command-line linker options, including any flags "
             + "inferred from the command-line options."
   )
-  public ImmutableList<String> getLinkOptions() {
-    return getLinkOptions(nonConfiguredSysroot);
+  public ImmutableList<String> getLinkOptionsWithLegacySysroot() {
+    return getLinkOptionsDoNotUse(nonConfiguredSysroot);
   }
 
-  public ImmutableList<String> getLinkOptions(PathFragment sysroot) {
+  /**
+   * @deprecated since it hardcodes --sysroot flag. Use
+   * {@link com.google.devtools.build.lib.rules.cpp.CcToolchainFeatures.FeatureConfiguration}
+   * instead.
+   */
+  // TODO(b/65401585): Migrate existing uses to cc_toolchain and cleanup here.
+  @Deprecated
+  ImmutableList<String> getLinkOptionsDoNotUse(@Nullable PathFragment sysroot) {
     if (sysroot == null) {
       return linkOptions;
     } else {
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionBuilder.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionBuilder.java
index d4f3e4e515..2d47b74687 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionBuilder.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionBuilder.java
@@ -1436,6 +1436,11 @@ public class CppLinkActionBuilder {
         buildVariables.addStringVariable(FORCE_PIC_VARIABLE, "");
       }
 
+      if (toolchain.getSysroot() != null) {
+        buildVariables.addStringVariable(
+            CppModel.SYSROOT_VARIABLE_NAME, toolchain.getSysroot().getPathString());
+      }
+
       if (cppConfiguration.shouldStripBinaries()) {
         buildVariables.addStringVariable(STRIP_DEBUG_SYMBOLS_VARIABLE, "");
       }
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppModel.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppModel.java
index fa1253f618..72b65a983f 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppModel.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppModel.java
@@ -159,7 +159,10 @@ public final class CppModel {
 
   /** Build variable for flags coming from unfiltered_cxx_flag CROSSTOOL fields. */
   public static final String UNFILTERED_COMPILE_FLAGS_VARIABLE_NAME = "unfiltered_compile_flags";
-  
+
+  /** Name of the build variable for the sysroot path variable name. */
+  public static final String SYSROOT_VARIABLE_NAME = "sysroot";
+
   private final CppSemantics semantics;
   private final RuleContext ruleContext;
   private final BuildConfiguration configuration;
@@ -641,6 +644,11 @@ public final class CppModel {
           LTO_INDEXING_BITCODE_FILE_VARIABLE_NAME, ltoIndexingFile.getExecPathString());
     }
 
+    if (ccToolchain.getSysroot() != null) {
+      buildVariables.addStringVariable(
+          SYSROOT_VARIABLE_NAME, ccToolchain.getSysroot().getPathString());
+    }
+
     buildVariables.addAllStringVariables(ccToolchain.getBuildVariables());
 
     buildVariables.addAllStringVariables(sourceSpecificBuildVariables);
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/LinkCommandLine.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/LinkCommandLine.java
index 1943cddf88..4d4a6c9286 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/LinkCommandLine.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/LinkCommandLine.java
@@ -547,16 +547,10 @@ public final class LinkCommandLine extends CommandLine {
       // Needed to find headers included from linkstamps.
       optionList.add("-I.");
 
-      // Add sysroot.
-      PathFragment sysroot = ccProvider.getSysroot();
-      if (sysroot != null) {
-        optionList.add("--sysroot=" + sysroot.getPathString());
-      }
-
       // Add toolchain compiler options.
       optionList.addAll(cppConfiguration.getCompilerOptions(features));
       optionList.addAll(cppConfiguration.getCOptions());
-      optionList.addAll(ccProvider.getUnfilteredCompilerOptions(features));
+      optionList.addAll(ccProvider.getUnfilteredCompilerOptionsWithSysroot(features));
       if (CppFileTypes.CPP_SOURCE.matches(linkstamp.getKey().getExecPath())) {
         optionList.addAll(cppConfiguration.getCxxOptions(features));
       }