Allow ios_test to override the GCOV environment variable

--
MOS_MIGRATED_REVID=88218457

9 files changed, 112 insertions, 8 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/OutputFileConfiguredTarget.java b/src/main/java/com/google/devtools/build/lib/analysis/OutputFileConfiguredTarget.java
index 51122e2af3..5380cf7ca9 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/OutputFileConfiguredTarget.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/OutputFileConfiguredTarget.java
@@ -15,12 +15,15 @@
 package com.google.devtools.build.lib.analysis;
 
 import com.google.common.base.Preconditions;
+import com.google.common.collect.ImmutableMap;
 import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.collect.nestedset.NestedSet;
 import com.google.devtools.build.lib.packages.OutputFile;
 import com.google.devtools.build.lib.rules.test.InstrumentedFilesProvider;
 import com.google.devtools.build.lib.rules.test.InstrumentedFilesProviderImpl;
 
+import java.util.Map;
+
 /**
  * A ConfiguredTarget for an OutputFile.
  */
@@ -64,6 +67,11 @@ public class OutputFileConfiguredTarget extends FileConfiguredTarget
         .getInstrumentationMetadataFiles();
   }
 
+  @Override
+  public Map<String, String> getExtraEnv() {
+    return ImmutableMap.of();
+  }
+
   /**
    * Returns the corresponding provider from the generating rule, if it is non-null, or {@code
    * defaultValue} otherwise.
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/RuleConfiguredTargetBuilder.java b/src/main/java/com/google/devtools/build/lib/analysis/RuleConfiguredTargetBuilder.java
index 9cd41c3609..376045c382 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/RuleConfiguredTargetBuilder.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/RuleConfiguredTargetBuilder.java
@@ -163,9 +163,16 @@ public final class RuleConfiguredTargetBuilder {
           "Having more than 50 shards is indicative of poor test organization. "
           + "Please reduce the number of shards.");
     }
-    final TestParams testParams = new TestActionBuilder(ruleContext)
+    TestActionBuilder testActionBuilder = new TestActionBuilder(ruleContext);
+    InstrumentedFilesProvider instrumentedFilesProvider =
+        findProvider(InstrumentedFilesProvider.class);
+    if (instrumentedFilesProvider != null) {
+      testActionBuilder
+          .setInstrumentedFiles(instrumentedFilesProvider)
+          .setExtraEnv(instrumentedFilesProvider.getExtraEnv());
+    }
+    final TestParams testParams = testActionBuilder
         .setFilesToRunProvider(filesToRunProvider)
-        .setInstrumentedFiles(findProvider(InstrumentedFilesProvider.class))
         .setExecutionRequirements(findProvider(ExecutionInfoProvider.class))
         .setShardCount(explicitShardCount)
         .build();
diff --git a/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcConfiguration.java b/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcConfiguration.java
index 3473e4b742..bc2b862cbd 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcConfiguration.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcConfiguration.java
@@ -19,10 +19,13 @@ import com.google.common.base.Preconditions;
 import com.google.common.collect.ImmutableList;
 import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
 import com.google.devtools.build.lib.analysis.config.CompilationMode;
+import com.google.devtools.build.lib.syntax.Label;
 import com.google.devtools.build.xcode.common.Platform;
 
 import java.util.List;
 
+import javax.annotation.Nullable;
+
 /**
  * A compiler configuration containing flags required for Objective-C compilation.
  */
@@ -50,7 +53,18 @@ public class ObjcConfiguration extends BuildConfiguration.Fragment {
   private final List<String> copts;
   private final CompilationMode compilationMode;
 
-  ObjcConfiguration(ObjcCommandLineOptions objcOptions, BuildConfiguration.Options options) {
+  // We only load this label if coverage mode is enabled. That is know as part of the
+  // BuildConfiguration. This label needs to be part of a configuration because only configurations
+  // can conditionally cause loading.
+  // This is referenced from a late bound attribute, and if loading wasn't forced in a
+  // configuration, the late bound attribute will fail to be initialized because it hasn't been
+  // loaded.
+  @Nullable private final Label gcovLabel;
+
+  ObjcConfiguration(
+      ObjcCommandLineOptions objcOptions,
+      BuildConfiguration.Options options,
+      @Nullable Label gcovLabel) {
     this.iosSdkVersion = Preconditions.checkNotNull(objcOptions.iosSdkVersion, "iosSdkVersion");
     this.iosMinimumOs = Preconditions.checkNotNull(objcOptions.iosMinimumOs, "iosMinimumOs");
     this.iosSimulatorVersion =
@@ -61,6 +75,7 @@ public class ObjcConfiguration extends BuildConfiguration.Fragment {
     this.runMemleaks = objcOptions.runMemleaks;
     this.copts = ImmutableList.copyOf(objcOptions.copts);
     this.compilationMode = Preconditions.checkNotNull(options.compilationMode, "compilationMode");
+    this.gcovLabel = gcovLabel;
   }
 
   public String getIosSdkVersion() {
@@ -131,6 +146,14 @@ public class ObjcConfiguration extends BuildConfiguration.Fragment {
     return copts;
   }
 
+  /**
+   * Returns the label of the gcov binary, used to get test coverage data. Null iff not in coverage
+   * mode.
+   */
+  @Nullable public Label getGcovLabel() {
+    return gcovLabel;
+  }
+
   @Override
   public String getName() {
     return "Objective-C";
diff --git a/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcConfigurationLoader.java b/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcConfigurationLoader.java
index 19713a346a..58b4f175c9 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcConfigurationLoader.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcConfigurationLoader.java
@@ -15,10 +15,14 @@
 package com.google.devtools.build.lib.rules.objc;
 
 import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
+import com.google.devtools.build.lib.analysis.config.BuildConfiguration.Options;
 import com.google.devtools.build.lib.analysis.config.BuildOptions;
 import com.google.devtools.build.lib.analysis.config.ConfigurationEnvironment;
 import com.google.devtools.build.lib.analysis.config.ConfigurationFragmentFactory;
 import com.google.devtools.build.lib.analysis.config.InvalidConfigurationException;
+import com.google.devtools.build.lib.packages.NoSuchPackageException;
+import com.google.devtools.build.lib.packages.NoSuchTargetException;
+import com.google.devtools.build.lib.syntax.Label;
 
 /**
  * A loader that creates ObjcConfiguration instances based on Objective-C configurations and
@@ -28,8 +32,21 @@ public class ObjcConfigurationLoader implements ConfigurationFragmentFactory {
   @Override
   public ObjcConfiguration create(ConfigurationEnvironment env, BuildOptions buildOptions)
       throws InvalidConfigurationException {
+    Options options = buildOptions.get(BuildConfiguration.Options.class);
+    Label gcovLabel = null;
+    if (options.collectCodeCoverage) {
+      try {
+        // TODO(danielwh): Replace this with something from an objc_toolchain when it exists
+        gcovLabel = Label.parseAbsolute("//third_party/gcov:gcov_for_xcode");
+        // Force the label to be loaded
+        env.getTarget(gcovLabel);
+      } catch (Label.SyntaxException | NoSuchPackageException | NoSuchTargetException e) {
+        throw new InvalidConfigurationException("Error parsing or loading objc coverage label: "
+            + e.getMessage(), e);
+      }
+    }
     return new ObjcConfiguration(buildOptions.get(ObjcCommandLineOptions.class),
-        buildOptions.get(BuildConfiguration.Options.class));
+        options, gcovLabel);
   }
 
   @Override
diff --git a/src/main/java/com/google/devtools/build/lib/rules/test/InstrumentedFilesProvider.java b/src/main/java/com/google/devtools/build/lib/rules/test/InstrumentedFilesProvider.java
index b1f956c54c..166c7e27aa 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/test/InstrumentedFilesProvider.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/test/InstrumentedFilesProvider.java
@@ -18,6 +18,8 @@ import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.analysis.TransitiveInfoProvider;
 import com.google.devtools.build.lib.collect.nestedset.NestedSet;
 
+import java.util.Map;
+
 /**
  * A provider of instrumented file sources and instrumentation metadata.
  */
@@ -32,4 +34,9 @@ public interface InstrumentedFilesProvider extends TransitiveInfoProvider {
    * Returns a collection of instrumentation metadata files.
    */
   NestedSet<Artifact> getInstrumentationMetadataFiles();
+
+  /**
+   * Returns environment variables which should be set for coverage to function.
+   */
+  Map<String, String> getExtraEnv();
 }
diff --git a/src/main/java/com/google/devtools/build/lib/rules/test/InstrumentedFilesProviderImpl.java b/src/main/java/com/google/devtools/build/lib/rules/test/InstrumentedFilesProviderImpl.java
index ad5f333197..5bf2155a55 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/test/InstrumentedFilesProviderImpl.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/test/InstrumentedFilesProviderImpl.java
@@ -13,11 +13,14 @@
 // limitations under the License.
 package com.google.devtools.build.lib.rules.test;
 
+import com.google.common.collect.ImmutableMap;
 import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.collect.nestedset.NestedSet;
 import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
 import com.google.devtools.build.lib.collect.nestedset.Order;
 
+import java.util.Map;
+
 /**
  * An implementation class for the InstrumentedFilesProvider interface.
  */
@@ -31,20 +34,27 @@ public final class InstrumentedFilesProviderImpl implements InstrumentedFilesPro
     public NestedSet<Artifact> getInstrumentationMetadataFiles() {
       return NestedSetBuilder.<Artifact>emptySet(Order.STABLE_ORDER);
     }
+    @Override
+    public Map<String, String> getExtraEnv() {
+      return ImmutableMap.of();
+    }
   };
 
   private final NestedSet<Artifact> instrumentedFiles;
   private final NestedSet<Artifact> instrumentationMetadataFiles;
+  private final ImmutableMap<String, String> extraEnv;
 
   public InstrumentedFilesProviderImpl(NestedSet<Artifact> instrumentedFiles,
-      NestedSet<Artifact> instrumentationMetadataFiles) {
+      NestedSet<Artifact> instrumentationMetadataFiles, Map<String, String> extraEnv) {
     this.instrumentedFiles = instrumentedFiles;
     this.instrumentationMetadataFiles = instrumentationMetadataFiles;
+    this.extraEnv = ImmutableMap.copyOf(extraEnv);
   }
 
   public InstrumentedFilesProviderImpl(InstrumentedFilesCollector collector) {
     this.instrumentedFiles = collector.getInstrumentedFiles();
     this.instrumentationMetadataFiles = collector.getInstrumentationMetadataFiles();
+    this.extraEnv = ImmutableMap.of();
   }
 
   @Override
@@ -56,4 +66,9 @@ public final class InstrumentedFilesProviderImpl implements InstrumentedFilesPro
   public NestedSet<Artifact> getInstrumentationMetadataFiles() {
     return instrumentationMetadataFiles;
   }
+
+  @Override
+  public Map<String, String> getExtraEnv() {
+    return extraEnv;
+  }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/rules/test/StandaloneTestStrategy.java b/src/main/java/com/google/devtools/build/lib/rules/test/StandaloneTestStrategy.java
index aff00873a7..fca0e6d462 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/test/StandaloneTestStrategy.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/test/StandaloneTestStrategy.java
@@ -120,7 +120,7 @@ public class StandaloneTestStrategy extends TestStrategy {
     BuildConfiguration config = action.getConfiguration();
 
     vars.putAll(config.getDefaultShellEnvironment());
-    vars.putAll(config.getTestEnv());
+    vars.putAll(action.getTestEnv());
     vars.put("TEST_SRCDIR", runfilesDir.getRelative(action.getRunfilesPrefix()).getPathString());
 
     // TODO(bazel-team): set TEST_TMPDIR.
diff --git a/src/main/java/com/google/devtools/build/lib/rules/test/TestActionBuilder.java b/src/main/java/com/google/devtools/build/lib/rules/test/TestActionBuilder.java
index 2b3b698828..46ba583db0 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/test/TestActionBuilder.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/test/TestActionBuilder.java
@@ -16,6 +16,7 @@ package com.google.devtools.build.lib.rules.test;
 
 import com.google.common.base.Preconditions;
 import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.Lists;
 import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.actions.Root;
@@ -39,6 +40,7 @@ import com.google.devtools.common.options.EnumConverter;
 
 import java.util.Collection;
 import java.util.List;
+import java.util.Map;
 
 import javax.annotation.Nullable;
 
@@ -53,9 +55,11 @@ public final class TestActionBuilder {
   private ExecutionInfoProvider executionRequirements;
   private InstrumentedFilesProvider instrumentedFiles;
   private int explicitShardCount;
+  private Map<String, String> extraEnv;
 
   public TestActionBuilder(RuleContext ruleContext) {
     this.ruleContext = ruleContext;
+    this.extraEnv = ImmutableMap.of();
   }
 
   /**
@@ -110,6 +114,12 @@ public final class TestActionBuilder {
     return this;
   }
 
+  public TestActionBuilder setExtraEnv(@Nullable Map<String, String> extraEnv) {
+    this.extraEnv = extraEnv == null
+        ? ImmutableMap.<String, String> of() : ImmutableMap.copyOf(extraEnv);
+    return this;
+  }
+
   /**
    * Set the explicit shard count. Note that this may be overridden by the sharding strategy.
    */
@@ -255,7 +265,7 @@ public final class TestActionBuilder {
             ruleContext.getActionOwner(), inputs,
             testLog, cacheStatus,
             coverageArtifact, microCoverageArtifact,
-            testProperties, executionSettings,
+            testProperties, extraEnv, executionSettings,
             shard, run, config, ruleContext.getWorkspaceName()));
         results.add(cacheStatus);
       }
diff --git a/src/main/java/com/google/devtools/build/lib/rules/test/TestRunnerAction.java b/src/main/java/com/google/devtools/build/lib/rules/test/TestRunnerAction.java
index 52fa09cc27..13ec62cedc 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/test/TestRunnerAction.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/test/TestRunnerAction.java
@@ -16,6 +16,7 @@ package com.google.devtools.build.lib.rules.test;
 
 import com.google.common.base.Preconditions;
 import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.Iterables;
 import com.google.devtools.build.lib.actions.AbstractAction;
 import com.google.devtools.build.lib.actions.ActionExecutionContext;
@@ -40,6 +41,8 @@ import com.google.devtools.common.options.TriState;
 import java.io.IOException;
 import java.io.InputStream;
 import java.io.OutputStream;
+import java.util.HashMap;
+import java.util.Map;
 import java.util.logging.Level;
 
 import javax.annotation.Nullable;
@@ -85,6 +88,8 @@ public class TestRunnerAction extends AbstractAction implements NotifyOnActionCa
   private boolean checkedCaching = false;
   private boolean unconditionalExecution = false;
 
+  private ImmutableMap<String, String> testEnv;
+
   private static ImmutableList<Artifact> list(Artifact... artifacts) {
     ImmutableList.Builder<Artifact> builder = ImmutableList.builder();
     for (Artifact artifact : artifacts) {
@@ -110,6 +115,7 @@ public class TestRunnerAction extends AbstractAction implements NotifyOnActionCa
       Artifact coverageArtifact,
       Artifact microCoverageArtifact,
       TestTargetProperties testProperties,
+      Map<String, String> extraTestEnv,
       TestTargetExecutionSettings executionSettings,
       int shardNum,
       int runNumber,
@@ -150,6 +156,10 @@ public class TestRunnerAction extends AbstractAction implements NotifyOnActionCa
     this.undeclaredOutputsAnnotationsPath = undeclaredOutputsAnnotationsDir.getChild("ANNOTATIONS");
     this.testInfrastructureFailure = baseDir.getChild(namePrefix + ".infrastructure_failure");
     this.workspaceName = workspaceName;
+
+    Map<String, String> mergedTestEnv = new HashMap<>(configuration.getTestEnv());
+    mergedTestEnv.putAll(extraTestEnv);
+    this.testEnv = ImmutableMap.copyOf(mergedTestEnv);
   }
 
   public BuildConfiguration getConfiguration() {
@@ -182,7 +192,7 @@ public class TestRunnerAction extends AbstractAction implements NotifyOnActionCa
     f.addString(executionSettings.getTestFilter() == null ? "" : executionSettings.getTestFilter());
     RunUnder runUnder = executionSettings.getRunUnder();
     f.addString(runUnder == null ? "" : runUnder.getValue());
-    f.addStringMap(configuration.getTestEnv());
+    f.addStringMap(getTestEnv());
     f.addString(testProperties.getSize().toString());
     f.addString(testProperties.getTimeout().toString());
     f.addStrings(testProperties.getTags());
@@ -389,6 +399,13 @@ public class TestRunnerAction extends AbstractAction implements NotifyOnActionCa
     return testLog;
   }
 
+  /**
+   * Returns all environment variables which must be set in order to run this test.
+   */
+  public Map<String, String> getTestEnv() {
+    return testEnv;
+  }
+
   public ResolvedPaths resolve(Path execRoot) {
     return new ResolvedPaths(execRoot);
   }