From 88fa594c780170e1443ef6e8f12916d95f6b9a9a Mon Sep 17 00:00:00 2001 From: Philipp Wollermann Date: Tue, 5 Jan 2016 13:35:26 +0000 Subject: Remove Constants.ALLOW_CC_INCLUDE_SCANNING and handle the logic in CppSemantics instead. -- MOS_MIGRATED_REVID=111406721 --- .../lib/bazel/rules/cpp/BazelCppSemantics.java | 16 +++++++++++- .../build/lib/rules/cpp/CcLibraryHelper.java | 6 ++--- .../build/lib/rules/cpp/CcToolchainProvider.java | 5 +--- .../lib/rules/cpp/CppCompileActionBuilder.java | 23 ++++++++++------- .../build/lib/rules/cpp/CppConfiguration.java | 30 ---------------------- .../devtools/build/lib/rules/cpp/CppHelper.java | 25 +++++++++--------- .../build/lib/rules/cpp/CppLinkAction.java | 3 --- .../devtools/build/lib/rules/cpp/CppOptions.java | 16 ------------ .../devtools/build/lib/rules/cpp/CppSemantics.java | 12 +++++++++ 9 files changed, 58 insertions(+), 78 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 df70c821f7..4c50b3a1ae 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 @@ -15,6 +15,7 @@ package com.google.devtools.build.lib.bazel.rules.cpp; import com.google.devtools.build.lib.actions.Artifact; +import com.google.devtools.build.lib.actions.Root; import com.google.devtools.build.lib.analysis.RuleContext; import com.google.devtools.build.lib.rules.cpp.CppCompilationContext.Builder; import com.google.devtools.build.lib.rules.cpp.CppCompileActionBuilder; @@ -44,7 +45,10 @@ public class BazelCppSemantics implements CppSemantics { RuleContext ruleContext, CppCompileActionBuilder actionBuilder) { actionBuilder.setCppConfiguration(ruleContext.getFragment(CppConfiguration.class)); actionBuilder.setActionContext(CppCompileActionContext.class); - actionBuilder.addTransitiveMandatoryInputs(CppHelper.getToolchain(ruleContext).getCompile()); + // Because Bazel does not support include scanning, we need the entire crosstool filegroup, + // including header files, as opposed to just the "compile" filegroup. + actionBuilder.addTransitiveMandatoryInputs(CppHelper.getToolchain(ruleContext).getCrosstool()); + actionBuilder.setShouldScanIncludes(false); } @Override @@ -55,4 +59,14 @@ public class BazelCppSemantics implements CppSemantics { public HeadersCheckingMode determineHeadersCheckingMode(RuleContext ruleContext) { return HeadersCheckingMode.STRICT; } + + @Override + public boolean needsIncludeScanning(RuleContext ruleContext) { + return false; + } + + @Override + public Root getGreppedIncludesDirectory(RuleContext ruleContext) { + return null; + } } diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLibraryHelper.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLibraryHelper.java index 0e05856495..280ad7b1f8 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLibraryHelper.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLibraryHelper.java @@ -764,11 +764,11 @@ public final class CcLibraryHelper { contextBuilder.addDeclaredIncludeSrcs(publicTextualHeaders); contextBuilder.addDeclaredIncludeSrcs(privateHeaders); contextBuilder.addPregreppedHeaderMap( - CppHelper.createExtractInclusions(ruleContext, publicHeaders)); + CppHelper.createExtractInclusions(ruleContext, semantics, publicHeaders)); contextBuilder.addPregreppedHeaderMap( - CppHelper.createExtractInclusions(ruleContext, publicTextualHeaders)); + CppHelper.createExtractInclusions(ruleContext, semantics, publicTextualHeaders)); contextBuilder.addPregreppedHeaderMap( - CppHelper.createExtractInclusions(ruleContext, privateHeaders)); + CppHelper.createExtractInclusions(ruleContext, semantics, privateHeaders)); contextBuilder.addCompilationPrerequisites(prerequisites); // Add this package's dir to declaredIncludeDirs, & this rule's headers to declaredIncludeSrcs 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 507880a953..6df75aac15 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 @@ -125,10 +125,7 @@ public final class CcToolchainProvider implements TransitiveInfoProvider { * Returns the files necessary for compilation. */ public NestedSet getCompile() { - // If include scanning is disabled, we need the entire crosstool filegroup, including header - // files. If it is enabled, we use the filegroup without header files - they are found by - // include scanning. For go, we also don't need the header files. - return cppConfiguration != null && cppConfiguration.shouldScanIncludes() ? compile : crosstool; + return compile; } /** 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 1edaa5e01f..a080d9ed37 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 CppConfiguration cppConfiguration; private ImmutableMap lipoScannableMap; private RuleContext ruleContext = null; + private Boolean shouldScanIncludes; // New fields need to be added to the copy constructor. /** @@ -167,6 +168,7 @@ public class CppCompileActionBuilder { this.usePic = other.usePic; this.lipoScannableMap = other.lipoScannableMap; this.ruleContext = other.ruleContext; + this.shouldScanIncludes = other.shouldScanIncludes; } public PathFragment getTempOutputFile() { @@ -245,18 +247,13 @@ public class CppCompileActionBuilder { * action). */ public CppCompileAction build() { + // This must be set either to false or true by CppSemantics, otherwise someone forgot to call + // finalizeCompileActionBuilder on this builder. + Preconditions.checkNotNull(shouldScanIncludes); + // Configuration can be null in tests. NestedSetBuilder realMandatoryInputsBuilder = NestedSetBuilder.compileOrder(); realMandatoryInputsBuilder.addTransitive(mandatoryInputsBuilder.build()); - String filename = sourceFile.getFilename(); - // Assembler without C preprocessing can use the '.include' pseudo-op which is not - // understood by the include scanner, so we'll disable scanning, and instead require - // the declared sources to state (possibly overapproximate) the dependencies. - // Assembler with preprocessing can also use '.include', but supporting both kinds - // of inclusion for that use-case is ridiculous. - boolean shouldScanIncludes = !CppFileTypes.ASSEMBLER.matches(filename) - && configuration != null - && configuration.getFragment(CppConfiguration.class).shouldScanIncludes(); if (tempOutputFile == null && !shouldScanIncludes) { realMandatoryInputsBuilder.addTransitive(context.getDeclaredIncludeSrcs()); } @@ -458,4 +455,12 @@ public class CppCompileActionBuilder { this.usePic = usePic; return this; } + + public void setShouldScanIncludes(boolean shouldScanIncludes) { + this.shouldScanIncludes = shouldScanIncludes; + } + + public boolean getShouldScanIncludes() { + return shouldScanIncludes; + } } 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 2f4c9e61e2..31aa2f0fee 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 @@ -25,7 +25,6 @@ import com.google.common.collect.Iterables; import com.google.common.collect.ListMultimap; import com.google.common.collect.Maps; import com.google.common.collect.Multimap; -import com.google.devtools.build.lib.Constants; import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.actions.ArtifactFactory; import com.google.devtools.build.lib.actions.PackageRootResolutionException; @@ -48,7 +47,6 @@ import com.google.devtools.build.lib.rules.cpp.CppConfigurationLoader.CppConfigu import com.google.devtools.build.lib.rules.cpp.FdoSupport.FdoException; import com.google.devtools.build.lib.skylarkinterface.SkylarkCallable; import com.google.devtools.build.lib.skylarkinterface.SkylarkModule; -import com.google.devtools.build.lib.util.IncludeScanningUtil; import com.google.devtools.build.lib.util.Preconditions; import com.google.devtools.build.lib.vfs.FileSystemUtils; import com.google.devtools.build.lib.vfs.Path; @@ -354,7 +352,6 @@ public class CppConfiguration extends BuildConfiguration.Fragment { * CppCompilationContexts, but registering build actions is disabled. */ private final boolean lipoContextCollector; - private final Root greppedIncludesDirectory; protected CppConfiguration(CppConfigurationParameters params) throws InvalidConfigurationException { @@ -372,11 +369,6 @@ public class CppConfiguration extends BuildConfiguration.Fragment { this.lipoContextCollector = cppOptions.lipoCollector; this.execRoot = params.execRoot; - // Note that the grepped includes directory is not configuration-specific; the paths of the - // files within that directory, however, are configuration-specific. - this.greppedIncludesDirectory = Root.asDerivedRoot(execRoot, - execRoot.getRelative(IncludeScanningUtil.GREPPED_INCLUDES)); - this.crosstoolTopPathFragment = crosstoolTop.getPackageIdentifier().getPathFragment(); try { @@ -935,13 +927,6 @@ public class CppConfiguration extends BuildConfiguration.Fragment { return pathPrefix.getRelative(path); } - /** - * Returns the configuration-independent grepped-includes directory. - */ - public Root getGreppedIncludesDirectory() { - return greppedIncludesDirectory; - } - @VisibleForTesting List configureLinkerOptions( CompilationMode compilationMode, LipoMode lipoMode, LinkingMode linkingMode, @@ -1459,10 +1444,6 @@ public class CppConfiguration extends BuildConfiguration.Fragment { return commandLineDefines.get(var); } - public boolean shouldScanIncludes() { - return Constants.ALLOW_CC_INCLUDE_SCANNING && cppOptions.scanIncludes; - } - /** * Returns the currently active LIPO compilation mode. */ @@ -1582,14 +1563,6 @@ public class CppConfiguration extends BuildConfiguration.Fragment { return cppOptions.forceIgnoreDashStatic; } - /** - * Returns true iff this build configuration requires inclusion extraction - * (for include scanning) in the action graph. - */ - public boolean needsIncludeScanning() { - return cppOptions.extractInclusions; - } - /** * Returns true if shared libraries must be compiled with position independent code * on this platform or in this configuration. @@ -1915,9 +1888,6 @@ public class CppConfiguration extends BuildConfiguration.Fragment { if (fdoSupport.getFdoRoot() != null) { roots.add(fdoSupport.getFdoRoot()); } - - // Grepped header includes; this root is not configuration specific. - roots.add(getGreppedIncludesDirectory()); } @Override diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppHelper.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppHelper.java index 16939f6466..94c689657c 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppHelper.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppHelper.java @@ -260,15 +260,15 @@ public class CppHelper { * performance problems if many headers are generated. */ @Nullable - public static final Map createExtractInclusions(RuleContext ruleContext, - Iterable prerequisites) { + public static final Map createExtractInclusions( + RuleContext ruleContext, CppSemantics semantics, Iterable prerequisites) { Map extractions = new HashMap<>(); for (Artifact prerequisite : prerequisites) { if (extractions.containsKey(prerequisite)) { // Don't create duplicate actions just because user specified same header file twice. continue; } - Artifact scanned = createExtractInclusions(ruleContext, prerequisite); + Artifact scanned = createExtractInclusions(ruleContext, semantics, prerequisite); if (scanned != null) { extractions.put(prerequisite, scanned); } @@ -284,13 +284,13 @@ public class CppHelper { * .cc source in serial. For high-latency file systems, this could cause * performance problems if many headers are generated. */ - private static final Artifact createExtractInclusions(RuleContext ruleContext, - Artifact prerequisite) { - if (ruleContext != null && - ruleContext.getFragment(CppConfiguration.class).needsIncludeScanning() && - !prerequisite.isSourceArtifact() && - CPP_FILETYPES.matches(prerequisite.getFilename())) { - Artifact scanned = getIncludesOutput(ruleContext, prerequisite); + private static final Artifact createExtractInclusions( + RuleContext ruleContext, CppSemantics semantics, Artifact prerequisite) { + if (ruleContext != null + && semantics.needsIncludeScanning(ruleContext) + && !prerequisite.isSourceArtifact() + && CPP_FILETYPES.matches(prerequisite.getFilename())) { + Artifact scanned = getIncludesOutput(ruleContext, semantics, prerequisite); ruleContext.registerAction( new ExtractInclusionAction(ruleContext.getActionOwner(), prerequisite, scanned)); return scanned; @@ -298,8 +298,9 @@ public class CppHelper { return null; } - private static Artifact getIncludesOutput(RuleContext ruleContext, Artifact src) { - Root root = ruleContext.getFragment(CppConfiguration.class).getGreppedIncludesDirectory(); + private static Artifact getIncludesOutput( + RuleContext ruleContext, CppSemantics semantics, Artifact src) { + Root root = semantics.getGreppedIncludesDirectory(ruleContext); PathFragment relOut = IncludeScanningUtil.getRootRelativeOutputPath(src.getExecPath()); return ruleContext.getShareableArtifact(relOut, root); } diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkAction.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkAction.java index bb7bd649a2..c4b240cfb8 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkAction.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkAction.java @@ -1119,9 +1119,6 @@ public final class CppLinkAction extends AbstractAction { this.linkstamps.addAll(linkstamps.keySet()); // Add inputs for linkstamping. if (!linkstamps.isEmpty()) { - // This will just be the compiler unless include scanning is disabled, in which case it will - // include all header files. Since we insist that linkstamps declare all their headers, all - // header files would be overkill, but that only happens when include scanning is disabled. addTransitiveCompilationInputs(toolchain.getCompile()); for (Map.Entry> entry : linkstamps.entrySet()) { addCompilationInputs(entry.getValue()); diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppOptions.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppOptions.java index ee1d3e077c..4733e741d0 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppOptions.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppOptions.java @@ -202,20 +202,6 @@ public class CppOptions extends FragmentOptions { "All ELF toolchains currently support this setting.") public boolean useInterfaceSharedObjects; - @Option(name = "cc_include_scanning", - defaultValue = "true", - category = "strategy", - help = "Whether to perform include scanning. Without it, your build will most likely " - + "fail.") - public boolean scanIncludes; - - @Option(name = "extract_generated_inclusions", - defaultValue = "true", - category = "undocumented", - help = "Run grep-includes actions (used for include scanning) over " + - "generated headers and sources.") - public boolean extractInclusions; - @Option(name = "fission", defaultValue = "no", converter = FissionOptionConverter.class, @@ -513,11 +499,9 @@ public class CppOptions extends FragmentOptions { host.useThinArchives = useThinArchives; host.useStartEndLib = useStartEndLib; - host.extractInclusions = extractInclusions; host.stripBinaries = StripMode.ALWAYS; host.fdoOptimize = null; host.lipoMode = LipoMode.OFF; - host.scanIncludes = scanIncludes; host.inmemoryDotdFiles = inmemoryDotdFiles; return host; 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 b09739d598..cc726a51f5 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 @@ -15,6 +15,7 @@ package com.google.devtools.build.lib.rules.cpp; import com.google.devtools.build.lib.actions.Artifact; +import com.google.devtools.build.lib.actions.Root; import com.google.devtools.build.lib.analysis.RuleContext; import com.google.devtools.build.lib.rules.cpp.CppConfiguration.HeadersCheckingMode; import com.google.devtools.build.lib.vfs.PathFragment; @@ -52,4 +53,15 @@ public interface CppSemantics { * Determines the applicable mode of headers checking for the passed in ruleContext. */ HeadersCheckingMode determineHeadersCheckingMode(RuleContext ruleContext); + + /** + * Returns true iff this build configuration requires inclusion extraction (for include scanning) + * in the action graph. + */ + boolean needsIncludeScanning(RuleContext ruleContext); + + /** + * Returns the configuration-independent grepped-includes directory. + */ + Root getGreppedIncludesDirectory(RuleContext ruleContext); } -- cgit v1.2.3