Skip to content

Commit

Permalink
stop passing BuildConfiguration to CppCompileAction
Browse files Browse the repository at this point in the history
This fixes Ulf's TODO by adding fields for the local shell environment and code
coverage and as stated on it accessing anything other than these fields can
impact correctness.

--
Change-Id: I9ccebaa0bc8ed920e2941b3d156692cb1e0fe117
Reviewed-on: https://bazel-review.googlesource.com/c/6870/
MOS_MIGRATED_REVID=137275206
  • Loading branch information
tfarina authored and laszlocsomor committed Oct 27, 2016
1 parent 8a8a7fc commit f41c252
Show file tree
Hide file tree
Showing 3 changed files with 12 additions and 11 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,6 @@
import com.google.devtools.build.lib.actions.extra.ExtraActionInfo;
import com.google.devtools.build.lib.analysis.RuleContext;
import com.google.devtools.build.lib.analysis.actions.ExecutionInfoSpecifier;
import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
import com.google.devtools.build.lib.analysis.config.PerLabelOptions;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.collect.CollectionUtils;
Expand Down Expand Up @@ -164,10 +163,8 @@ public Collection<Artifact> getInputsForIncludedFile(
*/
public static final String CLIF_MATCH = "clif-match";

// 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;
private final ImmutableMap<String, String> localShellEnvironment;
private final boolean isCodeCoverageEnabled;
protected final Artifact outputFile;
private final Label sourceLabel;
private final Artifact optionalSourceFile;
Expand Down Expand Up @@ -271,7 +268,8 @@ protected CppCompileAction(
@Nullable Artifact gcnoFile,
@Nullable Artifact dwoFile,
Artifact optionalSourceFile,
BuildConfiguration configuration,
ImmutableMap<String, String> localShellEnvironment,
boolean isCodeCoverageEnabled,
CppConfiguration cppConfiguration,
CppCompilationContext context,
Class<? extends CppCompileActionContext> actionContext,
Expand Down Expand Up @@ -299,7 +297,8 @@ protected CppCompileAction(
lipoScannables),
CollectionUtils.asListWithoutNulls(
outputFile, (dotdFile == null ? null : dotdFile.artifact()), gcnoFile, dwoFile));
this.configuration = configuration;
this.localShellEnvironment = localShellEnvironment;
this.isCodeCoverageEnabled = isCodeCoverageEnabled;
this.sourceLabel = sourceLabel;
this.outputFile = Preconditions.checkNotNull(outputFile);
this.optionalSourceFile = optionalSourceFile;
Expand Down Expand Up @@ -677,8 +676,8 @@ public ImmutableCollection<String> getDefines() {

@Override
public ImmutableMap<String, String> getEnvironment() {
Map<String, String> environment = new LinkedHashMap<>(configuration.getLocalShellEnvironment());
if (configuration.isCodeCoverageEnabled()) {
Map<String, String> environment = new LinkedHashMap<>(localShellEnvironment);
if (isCodeCoverageEnabled) {
environment.put("PWD", "/proc/self/cwd");
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -330,7 +330,8 @@ public CppCompileAction build() {
gcnoFile,
dwoFile,
optionalSourceFile,
configuration,
configuration.getLocalShellEnvironment(),
configuration.isCodeCoverageEnabled(),
cppConfiguration,
context,
actionContext,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,8 @@ public class FakeCppCompileAction extends CppCompileAction {
null,
null,
null,
configuration,
configuration.getLocalShellEnvironment(),
configuration.isCodeCoverageEnabled(),
cppConfiguration,
// We only allow inclusion of header files explicitly declared in
// "srcs", so we only use declaredIncludeSrcs, not declaredIncludeDirs.
Expand Down

0 comments on commit f41c252

Please sign in to comment.