From 530876d5d6b80a94b0861a3684178cbb7f5072e8 Mon Sep 17 00:00:00 2001 From: Cal Peyser Date: Thu, 1 Sep 2016 14:02:58 +0000 Subject: Create injectable semantics for include validation, to allow it to be turned off in the objc case. -- MOS_MIGRATED_REVID=131943500 --- .../lib/bazel/rules/cpp/BazelCppSemantics.java | 5 ++ .../build/lib/rules/cpp/CppCompileAction.java | 56 ++++++++++++---------- .../lib/rules/cpp/CppCompileActionBuilder.java | 14 +++++- .../devtools/build/lib/rules/cpp/CppModel.java | 2 + .../devtools/build/lib/rules/cpp/CppSemantics.java | 3 ++ .../build/lib/rules/cpp/FakeCppCompileAction.java | 7 +-- .../build/lib/rules/objc/ObjcCppSemantics.java | 5 ++ 7 files changed, 62 insertions(+), 30 deletions(-) (limited to 'src/main/java/com/google') diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/cpp/BazelCppSemantics.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/cpp/BazelCppSemantics.java index 737e69e4c6..a037904d44 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/rules/cpp/BazelCppSemantics.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/cpp/BazelCppSemantics.java @@ -67,4 +67,9 @@ public class BazelCppSemantics implements CppSemantics { @Override public void validateAttributes(RuleContext ruleContext) { } + + @Override + public boolean needsIncludeValidation() { + return true; + } } 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 0debf56da0..308b7a0686 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 @@ -176,6 +176,7 @@ public class CppCompileAction extends AbstractAction private final FeatureConfiguration featureConfiguration; protected final Class actionContext; private final SpecialInputsHandler specialInputsHandler; + private final CppSemantics semantics; /** * Identifier for the actual execution time behavior of the action. @@ -202,32 +203,29 @@ public class CppCompileAction extends AbstractAction /** * Creates a new action to compile C/C++ source files. * - * @param owner the owner of the action, usually the configured target that - * emitted it - * @param sourceFile the source file that should be compiled. {@code mandatoryInputs} must - * contain this file - * @param shouldScanIncludes a boolean indicating whether scanning of {@code sourceFile} - * is to be performed looking for inclusions. + * @param owner the owner of the action, usually the configured target that emitted it + * @param sourceFile the source file that should be compiled. {@code mandatoryInputs} must contain + * this file + * @param shouldScanIncludes a boolean indicating whether scanning of {@code sourceFile} is to be + * performed looking for inclusions. * @param sourceLabel the label of the rule the source file is generated by - * @param mandatoryInputs any additional files that need to be present for the - * compilation to succeed, can be empty but not null, for example, extra sources for FDO. - * @param outputFile the object file that is written as result of the - * compilation, or the fake object for {@link FakeCppCompileAction}s - * @param dotdFile the .d file that is generated as a side-effect of - * compilation - * @param gcnoFile the coverage notes that are written in coverage mode, can - * be null - * @param dwoFile the .dwo output file where debug information is stored for Fission - * builds (null if Fission mode is disabled) + * @param mandatoryInputs any additional files that need to be present for the compilation to + * succeed, can be empty but not null, for example, extra sources for FDO. + * @param outputFile the object file that is written as result of the compilation, or the fake + * object for {@link FakeCppCompileAction}s + * @param dotdFile the .d file that is generated as a side-effect of compilation + * @param gcnoFile the coverage notes that are written in coverage mode, can be null + * @param dwoFile the .dwo output file where debug information is stored for Fission builds (null + * if Fission mode is disabled) * @param optionalSourceFile an additional optional source file (null if unneeded) * @param configuration the build configurations * @param context the compilation context * @param copts options for the compiler * @param coptsFilter regular expression to remove options from {@code copts} * @param executionRequirements out-of-band hints to be passed to the execution backend to signal - * platform requirements + * platform requirements * @param actionName a string giving the name of this action for the purpose of toolchain - * evaluation + * evaluation */ protected CppCompileAction( ActionOwner owner, @@ -258,7 +256,8 @@ public class CppCompileAction extends AbstractAction ImmutableSet executionRequirements, ImmutableMap environment, String actionName, - RuleContext ruleContext) { + RuleContext ruleContext, + CppSemantics semantics) { super( owner, createInputs( @@ -297,6 +296,7 @@ public class CppCompileAction extends AbstractAction // artifact and will definitely exist prior to this action execution. this.mandatoryInputs = mandatoryInputs; this.builtinIncludeFiles = CppHelper.getToolchain(ruleContext).getBuiltinIncludeFiles(); + this.semantics = semantics; verifyIncludePaths(ruleContext); } @@ -994,7 +994,10 @@ public class CppCompileAction extends AbstractAction problems.add(execPathFragment.getPathString()); } } - problems.assertProblemFree(this, getSourceFile()); + //TODO(b/22551695): Remove in favor of seperate implementations. + if (semantics.needsIncludeValidation()) { + problems.assertProblemFree(this, getSourceFile()); + } } catch (IOException e) { // Some kind of IO or parse exception--wrap & rethrow it to stop the build. throw new ActionExecutionException("error while parsing .d file", e, this, false); @@ -1196,11 +1199,14 @@ public class CppCompileAction extends AbstractAction updateActionInputs(discoveredInputs); } - // hdrs_check: This cannot be switched off, because doing so would allow for incorrect builds. - validateInclusions( - discoveredInputs, - actionExecutionContext.getArtifactExpander(), - executor.getEventHandler()); + // hdrs_check: Turning this off opens the door to incorrect builds. However, we allow it + // to accommodate the current behavior in the objc rules. + if (semantics.needsIncludeValidation()) { + validateInclusions( + discoveredInputs, + actionExecutionContext.getArtifactExpander(), + executor.getEventHandler()); + } } /** 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 0fe142b181..260cc66b37 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 @@ -79,6 +79,7 @@ public class CppCompileActionBuilder { private RuleContext ruleContext = null; private Boolean shouldScanIncludes; private Map environment = new LinkedHashMap<>(); + private CppSemantics cppSemantics; // New fields need to be added to the copy constructor. /** @@ -147,6 +148,7 @@ public class CppCompileActionBuilder { this.ruleContext = other.ruleContext; this.shouldScanIncludes = other.shouldScanIncludes; this.environment = new LinkedHashMap<>(other.environment); + this.cppSemantics = other.cppSemantics; } public PathFragment getTempOutputFile() { @@ -289,7 +291,8 @@ public class CppCompileActionBuilder { actionContext, ImmutableList.copyOf(copts), getNocoptPredicate(nocopts), - ruleContext); + ruleContext, + cppSemantics); } else { NestedSet realMandatoryInputs = realMandatoryInputsBuilder.build(); @@ -320,7 +323,8 @@ public class CppCompileActionBuilder { executionRequirements, ImmutableMap.copyOf(environment), getActionName(), - ruleContext); + ruleContext, + cppSemantics); } } @@ -481,6 +485,12 @@ public class CppCompileActionBuilder { return this; } + /** Sets the CppSemantics for this compile. */ + public CppCompileActionBuilder setSemantics(CppSemantics semantics) { + this.cppSemantics = semantics; + return this; + } + public void setShouldScanIncludes(boolean shouldScanIncludes) { this.shouldScanIncludes = shouldScanIncludes; } 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 432dc3ca15..798d90998b 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 @@ -520,6 +520,8 @@ public final class CppModel { CppCompileActionBuilder builder = initializeCompileAction(sourceArtifact, sourceLabel, /*forInterface=*/ false); + builder.setSemantics(semantics); + if (CppFileTypes.CPP_HEADER.matches(source.getSource().getExecPath())) { createHeaderAction(outputName, result, env, builder, CppFileTypes.mustProduceDotdFile(sourceArtifact.getFilename())); diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppSemantics.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppSemantics.java index 62f0e69da9..ec224b2d34 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppSemantics.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppSemantics.java @@ -60,4 +60,7 @@ public interface CppSemantics { boolean needsIncludeScanning(RuleContext ruleContext); void validateAttributes(RuleContext ruleContext); + + /** Returns true iff this build requires include validation. */ + boolean needsIncludeValidation(); } 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 4b68bcb229..e82f352ce9 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 @@ -42,7 +42,6 @@ import com.google.devtools.build.lib.util.ShellEscaper; import com.google.devtools.build.lib.util.io.FileOutErr; import com.google.devtools.build.lib.vfs.FileSystemUtils; import com.google.devtools.build.lib.vfs.PathFragment; - import java.io.IOException; import java.util.UUID; import java.util.logging.Logger; @@ -78,7 +77,8 @@ public class FakeCppCompileAction extends CppCompileAction { Class actionContext, ImmutableList copts, Predicate nocopts, - RuleContext ruleContext) { + RuleContext ruleContext, + CppSemantics semantics) { super( owner, features, @@ -113,7 +113,8 @@ public class FakeCppCompileAction extends CppCompileAction { ImmutableSet.of(), ImmutableMap.of(), CppCompileAction.CPP_COMPILE, - ruleContext); + ruleContext, + semantics); this.tempOutputFile = Preconditions.checkNotNull(tempOutputFile); } diff --git a/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcCppSemantics.java b/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcCppSemantics.java index effd78fe7f..7e408c4bb9 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcCppSemantics.java +++ b/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcCppSemantics.java @@ -86,4 +86,9 @@ public class ObjcCppSemantics implements CppSemantics { @Override public void validateAttributes(RuleContext ruleContext) { } + + @Override + public boolean needsIncludeValidation() { + return false; + } } -- cgit v1.2.3