Remove the dependency from the C++ rules to Apple stuff.

This makes the C++ rules standalone, and the CcCommonTest ensures that (at
least) analysis of C++ targets works even if no Apple / Xcode rules are
present.

We can also compile them separately, in a future change.

--
MOS_MIGRATED_REVID=131583691

6 files changed, 49 insertions, 23 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchain.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchain.java
index f24432db98..f00c3d9d96 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchain.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchain.java
@@ -218,7 +218,8 @@ public class CcToolchain implements RuleConfiguredTargetFactory {
             supportsHeaderParsing,
             getBuildVariables(ruleContext),
             getBuiltinIncludes(ruleContext),
-            coverageEnvironment.build());
+            coverageEnvironment.build(),
+            getEnvironment(ruleContext));
     RuleConfiguredTargetBuilder builder =
         new RuleConfiguredTargetBuilder(ruleContext)
             .add(CcToolchainProvider.class, provider)
@@ -323,10 +324,22 @@ public class CcToolchain implements RuleConfiguredTargetFactory {
   }
 
   /**
-   * Returns a map that should be templated into the crosstool as build variables.  Is meant to
+   * Returns a map that should be templated into the crosstool as build variables. Is meant to
    * be overridden by subclasses of CcToolchain.
+   *
+   * @param ruleContext the rule context
    */
   protected Map<String, String> getBuildVariables(RuleContext ruleContext) {
     return ImmutableMap.<String, String>of();
   }
+
+  /**
+   * Returns a map of environment variables to be added to the compile actions created for this
+   * toolchain. Ideally, this will get replaced by features, which also allow setting env variables.
+   *
+   * @param ruleContext the rule context
+   */
+  protected ImmutableMap<String, String> getEnvironment(RuleContext ruleContext) {
+    return ImmutableMap.<String, String>of();
+  }
 }
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 082d469810..755da5311c 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
@@ -59,7 +59,8 @@ public final class CcToolchainProvider implements TransitiveInfoProvider {
           false,
           ImmutableMap.<String, String>of(),
           ImmutableList.<Artifact>of(),
-          NestedSetBuilder.<Pair<String, String>>emptySet(Order.COMPILE_ORDER));
+          NestedSetBuilder.<Pair<String, String>>emptySet(Order.COMPILE_ORDER),
+          ImmutableMap.<String, String>of());
 
   @Nullable private final CppConfiguration cppConfiguration;
   private final NestedSet<Artifact> crosstool;
@@ -78,9 +79,10 @@ public final class CcToolchainProvider implements TransitiveInfoProvider {
   private final CppCompilationContext cppCompilationContext;
   private final boolean supportsParamFiles;
   private final boolean supportsHeaderParsing;
-  private final Map<String, String> buildVariables;
+  private final ImmutableMap<String, String> buildVariables;
   private final ImmutableList<Artifact> builtinIncludeFiles;
   private final NestedSet<Pair<String, String>> coverageEnvironment;
+  private final ImmutableMap<String, String> environment;
 
   public CcToolchainProvider(
       @Nullable CppConfiguration cppConfiguration,
@@ -102,7 +104,8 @@ public final class CcToolchainProvider implements TransitiveInfoProvider {
       boolean supportsHeaderParsing,
       Map<String, String> buildVariables,
       ImmutableList<Artifact> builtinIncludeFiles,
-      NestedSet<Pair<String, String>> coverageEnvironment) {
+      NestedSet<Pair<String, String>> coverageEnvironment,
+      ImmutableMap<String, String> environment) {
     this.cppConfiguration = cppConfiguration;
     this.crosstool = Preconditions.checkNotNull(crosstool);
     this.crosstoolMiddleman = Preconditions.checkNotNull(crosstoolMiddleman);
@@ -120,9 +123,10 @@ public final class CcToolchainProvider implements TransitiveInfoProvider {
     this.cppCompilationContext = Preconditions.checkNotNull(cppCompilationContext);
     this.supportsParamFiles = supportsParamFiles;
     this.supportsHeaderParsing = supportsHeaderParsing;
-    this.buildVariables = buildVariables;
+    this.buildVariables = ImmutableMap.copyOf(buildVariables);
     this.builtinIncludeFiles = builtinIncludeFiles;
     this.coverageEnvironment = coverageEnvironment;
+    this.environment = environment;
   }
 
   /**
@@ -257,7 +261,7 @@ public final class CcToolchainProvider implements TransitiveInfoProvider {
   /**
    * Returns build variables to be templated into the crosstool.
    */
-  public Map<String, String> getBuildVariables() {
+  public ImmutableMap<String, String> getBuildVariables() {
     return buildVariables;
   }
 
@@ -275,4 +279,8 @@ public final class CcToolchainProvider implements TransitiveInfoProvider {
   public NestedSet<Pair<String, String>> getCoverageEnvironment() {
     return coverageEnvironment;
   }
+
+  public ImmutableMap<String, String> getEnvironment() {
+    return environment;
+  }
 }
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 456df2d61c..0debf56da0 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
@@ -51,8 +51,6 @@ import com.google.devtools.build.lib.events.Event;
 import com.google.devtools.build.lib.events.EventHandler;
 import com.google.devtools.build.lib.profiler.Profiler;
 import com.google.devtools.build.lib.profiler.ProfilerTask;
-import com.google.devtools.build.lib.rules.apple.AppleConfiguration;
-import com.google.devtools.build.lib.rules.apple.Platform;
 import com.google.devtools.build.lib.rules.cpp.CcToolchainFeatures.FeatureConfiguration;
 import com.google.devtools.build.lib.rules.cpp.CppCompileActionContext.Reply;
 import com.google.devtools.build.lib.rules.cpp.CppConfiguration.Tool;
@@ -157,7 +155,9 @@ public class CppCompileAction extends AbstractAction
   public static final String ASSEMBLE = "assemble";
   public static final String PREPROCESS_ASSEMBLE = "preprocess-assemble";
 
-
+  // TODO(ulfjack): this is only used to get the local shell environment and to check if coverage is
+  // enabled. Move those two things to local fields and drop this. Accessing anything other than
+  // these fields can impact correctness!
   private final BuildConfiguration configuration;
   protected final Artifact outputFile;
   private final Label sourceLabel;
@@ -170,6 +170,7 @@ public class CppCompileAction extends AbstractAction
   private final ImmutableList<Artifact> builtinIncludeFiles;
   @VisibleForTesting public final CppCompileCommandLine cppCompileCommandLine;
   private final ImmutableSet<String> executionRequirements;
+  private final ImmutableMap<String, String> environment;
 
   @VisibleForTesting final CppConfiguration cppConfiguration;
   private final FeatureConfiguration featureConfiguration;
@@ -255,6 +256,7 @@ public class CppCompileAction extends AbstractAction
       Iterable<IncludeScannable> lipoScannables,
       UUID actionClassId,
       ImmutableSet<String> executionRequirements,
+      ImmutableMap<String, String> environment,
       String actionName,
       RuleContext ruleContext) {
     super(
@@ -289,6 +291,7 @@ public class CppCompileAction extends AbstractAction
     this.lipoScannables = lipoScannables;
     this.actionClassId = actionClassId;
     this.executionRequirements = executionRequirements;
+    this.environment = environment;
 
     // We do not need to include the middleman artifact since it is a generated
     // artifact and will definitely exist prior to this action execution.
@@ -632,18 +635,7 @@ public class CppCompileAction extends AbstractAction
       environment.put("PWD", "/proc/self/cwd");
     }
 
-    // TODO(bazel-team): Handle at the level of crosstool (feature) templates instead of in this
-    // compile action. This will also prevent the need for apple host system and target platform
-    // evaluation here.
-    AppleConfiguration appleConfiguration = configuration.getFragment(AppleConfiguration.class);
-    if (CppConfiguration.MAC_SYSTEM_NAME.equals(getHostSystemName())) {
-      environment.putAll(appleConfiguration.getAppleHostSystemEnv());
-    }
-    if (Platform.isApplePlatform(cppConfiguration.getTargetCpu())) {
-      environment.putAll(appleConfiguration.appleTargetPlatformEnv(
-          Platform.forTargetCpu(cppConfiguration.getTargetCpu())));
-    }
-    
+    environment.putAll(this.environment);
     environment.putAll(cppCompileCommandLine.getEnvironment());
 
     // TODO(bazel-team): Check (crosstool) host system name instead of using OS.getCurrent.
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileActionBuilder.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileActionBuilder.java
index d5bd8ed5be..0fe142b181 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileActionBuilder.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileActionBuilder.java
@@ -38,7 +38,9 @@ import com.google.devtools.build.lib.util.Preconditions;
 import com.google.devtools.build.lib.vfs.PathFragment;
 import java.util.ArrayList;
 import java.util.Collection;
+import java.util.LinkedHashMap;
 import java.util.List;
+import java.util.Map;
 import java.util.UUID;
 import java.util.regex.Pattern;
 
@@ -76,6 +78,7 @@ public class CppCompileActionBuilder {
   private ImmutableMap<Artifact, IncludeScannable> lipoScannableMap;
   private RuleContext ruleContext = null;
   private Boolean shouldScanIncludes;
+  private Map<String, String> environment = new LinkedHashMap<>();
   // New fields need to be added to the copy constructor.
 
   /**
@@ -143,6 +146,7 @@ public class CppCompileActionBuilder {
     this.lipoScannableMap = other.lipoScannableMap;
     this.ruleContext = other.ruleContext;
     this.shouldScanIncludes = other.shouldScanIncludes;
+    this.environment = new LinkedHashMap<>(other.environment);
   }
 
   public PathFragment getTempOutputFile() {
@@ -263,7 +267,7 @@ public class CppCompileActionBuilder {
       executionRequirements =
           featureConfiguration.getToolForAction(getActionName()).getExecutionRequirements();
     }
-    
+
     // Copying the collections is needed to make the builder reusable.
     if (fake) {
       return new FakeCppCompileAction(
@@ -314,6 +318,7 @@ public class CppCompileActionBuilder {
           getLipoScannables(realMandatoryInputs),
           actionClassId,
           executionRequirements,
+          ImmutableMap.copyOf(environment),
           getActionName(),
           ruleContext);
     }
@@ -336,6 +341,11 @@ public class CppCompileActionBuilder {
     return this;
   }
 
+  public CppCompileActionBuilder addEnvironment(Map<String, String> environment) {
+    this.environment.putAll(environment);
+    return this;
+  }
+
   public CppCompileActionBuilder setSpecialInputsHandler(
       SpecialInputsHandler specialInputsHandler) {
     this.specialInputsHandler = specialInputsHandler;
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 f4c8e09a04..432dc3ca15 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
@@ -1015,6 +1015,7 @@ public final class CppModel {
         ruleContext, source, label);
 
     builder.setContext(forInterface ? interfaceContext : context).addCopts(copts);
+    builder.addEnvironment(CppHelper.getToolchain(ruleContext).getEnvironment());
     return builder;
   }
 
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/FakeCppCompileAction.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/FakeCppCompileAction.java
index e31ff2ee4b..4b68bcb229 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/FakeCppCompileAction.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/FakeCppCompileAction.java
@@ -20,6 +20,7 @@ import com.google.common.base.Function;
 import com.google.common.base.Joiner;
 import com.google.common.base.Predicate;
 import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Iterables;
 import com.google.devtools.build.lib.actions.ActionExecutionContext;
@@ -110,6 +111,7 @@ public class FakeCppCompileAction extends CppCompileAction {
         ImmutableList.<IncludeScannable>of(),
         GUID,
         ImmutableSet.<String>of(),
+        ImmutableMap.<String, String>of(),
         CppCompileAction.CPP_COMPILE,
         ruleContext);
     this.tempOutputFile = Preconditions.checkNotNull(tempOutputFile);