diff options
author | 2016-09-05 15:00:14 +0000 | |
---|---|---|
committer | 2016-09-06 15:38:51 +0000 | |
commit | c479a8eb53da5581bb2407d826085bf6bfd02779 (patch) | |
tree | 76a883692a602b9fb1afa2cdff8235b49a87542f /src/main/java/com | |
parent | 14a377d6f55f06dc5985e4701a2757c1c7b016d5 (diff) |
Rollback of commit 4689c5d1d2faf902846b100b8d858d172a0ceb3d.
*** Reason for rollback ***
Prunes .modulemap files incorrectly. See [].
*** Original change description ***
Implement input pruning using .d files in objc.
--
MOS_MIGRATED_REVID=132246906
Diffstat (limited to 'src/main/java/com')
10 files changed, 171 insertions, 679 deletions
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/actions/LTOBackendAction.java b/src/main/java/com/google/devtools/build/lib/analysis/actions/LTOBackendAction.java index 06dd6e8c8a..c5dc74489d 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/actions/LTOBackendAction.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/actions/LTOBackendAction.java @@ -25,7 +25,6 @@ import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.actions.ArtifactResolver; import com.google.devtools.build.lib.actions.PackageRootResolutionException; import com.google.devtools.build.lib.actions.PackageRootResolver; -import com.google.devtools.build.lib.actions.ResourceSet; import com.google.devtools.build.lib.collect.nestedset.NestedSet; import com.google.devtools.build.lib.util.Fingerprint; import com.google.devtools.build.lib.vfs.FileSystemUtils; @@ -229,12 +228,11 @@ public final class LTOBackendAction extends SpawnAction { } @Override - protected SpawnAction createSpawnAction( + SpawnAction createSpawnAction( ActionOwner owner, NestedSet<Artifact> tools, NestedSet<Artifact> inputsAndTools, ImmutableList<Artifact> outputs, - ResourceSet resourceSet, CommandLine actualCommandLine, ImmutableMap<String, String> env, ImmutableSet<String> clientEnvironmentVariables, diff --git a/src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnAction.java b/src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnAction.java index 999f094430..2a797237ff 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnAction.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnAction.java @@ -65,9 +65,7 @@ import javax.annotation.Nullable; /** An Action representing an arbitrary subprocess to be forked and exec'd. */ public class SpawnAction extends AbstractAction implements ExecutionInfoSpecifier, CommandAction { - - /** Sets extensions on ExtraActionInfo **/ - protected static class ExtraActionInfoSupplier<T> { + private static class ExtraActionInfoSupplier<T> { private final GeneratedExtension<ExtraActionInfo, T> extension; private final T value; @@ -487,7 +485,7 @@ public class SpawnAction extends AbstractAction implements ExecutionInfoSpecifie private ImmutableMap<String, String> executionInfo = ImmutableMap.of(); private boolean isShellCommand = false; private boolean useDefaultShellEnvironment = false; - protected boolean executeUnconditionally; + private boolean executeUnconditionally; private PathFragment executable; // executableArgs does not include the executable itself. private List<String> executableArgs; @@ -497,7 +495,7 @@ public class SpawnAction extends AbstractAction implements ExecutionInfoSpecifie private String progressMessage; private ParamFileInfo paramFileInfo = null; private String mnemonic = "Unknown"; - protected ExtraActionInfoSupplier<?> extraActionInfoSupplier = null; + private ExtraActionInfoSupplier<?> extraActionInfoSupplier = null; /** * Creates a SpawnAction builder. @@ -645,7 +643,6 @@ public class SpawnAction extends AbstractAction implements ExecutionInfoSpecifie tools, inputsAndTools, ImmutableList.copyOf(outputs), - resourceSet, actualCommandLine, ImmutableMap.copyOf(env), clientEnvironmentVariables, @@ -655,13 +652,11 @@ public class SpawnAction extends AbstractAction implements ExecutionInfoSpecifie mnemonic); } - /** Creates a SpawnAction. */ - protected SpawnAction createSpawnAction( + SpawnAction createSpawnAction( ActionOwner owner, NestedSet<Artifact> tools, NestedSet<Artifact> inputsAndTools, ImmutableList<Artifact> outputs, - ResourceSet resourceSet, CommandLine actualCommandLine, ImmutableMap<String, String> env, ImmutableSet<String> clientEnvironmentVariables, 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 5a284b314d..abc54a6efd 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 @@ -164,6 +164,7 @@ public class CppCompileAction extends AbstractAction private final Artifact optionalSourceFile; private final NestedSet<Artifact> mandatoryInputs; private final boolean shouldScanIncludes; + private final boolean shouldPruneModules; private final boolean usePic; private final CppCompilationContext context; private final Iterable<IncludeScannable> lipoScannables; @@ -175,8 +176,8 @@ public class CppCompileAction extends AbstractAction @VisibleForTesting final CppConfiguration cppConfiguration; private final FeatureConfiguration featureConfiguration; protected final Class<? extends CppCompileActionContext> actionContext; - protected final SpecialInputsHandler specialInputsHandler; - protected final CppSemantics cppSemantics; + private final SpecialInputsHandler specialInputsHandler; + private final CppSemantics semantics; /** * Identifier for the actual execution time behavior of the action. @@ -203,32 +204,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, @@ -239,6 +237,7 @@ public class CppCompileAction extends AbstractAction CcToolchainFeatures.Variables variables, Artifact sourceFile, boolean shouldScanIncludes, + boolean shouldPruneModules, boolean usePic, Label sourceLabel, NestedSet<Artifact> mandatoryInputs, @@ -260,7 +259,7 @@ public class CppCompileAction extends AbstractAction ImmutableMap<String, String> environment, String actionName, RuleContext ruleContext, - CppSemantics cppSemantics) { + CppSemantics semantics) { super( owner, createInputs( @@ -284,6 +283,7 @@ public class CppCompileAction extends AbstractAction // known after inclusion scanning. When *not* scanning includes, // the inputs are as declared, hence known, and remain so. this.shouldScanIncludes = shouldScanIncludes; + this.shouldPruneModules = shouldPruneModules; this.usePic = usePic; this.inputsKnown = !shouldScanIncludes; this.cppCompileCommandLine = @@ -299,7 +299,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.cppSemantics = cppSemantics; + this.semantics = semantics; verifyIncludePaths(ruleContext); } @@ -443,7 +443,7 @@ public class CppCompileAction extends AbstractAction return null; } - if (featureConfiguration.isEnabled(CppRuleClasses.PRUNE_HEADER_MODULES)) { + if (shouldPruneModules) { Set<Artifact> initialResultSet = Sets.newLinkedHashSet(initialResult); List<String> usedModulePaths = Lists.newArrayList(); for (Artifact usedModule : context.getUsedModules(usePic, initialResultSet)) { @@ -909,6 +909,105 @@ public class CppCompileAction extends AbstractAction } } + private DependencySet processDepset(Path execRoot, CppCompileActionContext.Reply reply) + throws IOException { + DotdFile dotdFile = getDotdFile(); + Preconditions.checkNotNull(dotdFile); + DependencySet depSet = new DependencySet(execRoot); + // artifact() is null if we are using in-memory .d files. We also want to prepare for the + // case where we expected an in-memory .d file, but we did not get an appropriate response. + // Perhaps we produced the file locally. + if (dotdFile.artifact() != null || reply == null) { + return depSet.read(dotdFile.getPath()); + } else { + // This is an in-memory .d file. + return depSet.process(reply.getContents()); + } + } + + /** + * Returns a collection with additional input artifacts relevant to the action by reading the + * dynamically-discovered dependency information from the .d file after the action has run. + * + * <p>Artifacts are considered inputs but not "mandatory" inputs. + * + * @param reply the reply from the compilation. + * @throws ActionExecutionException iff the .d is missing (when required), malformed, or has + * unresolvable included artifacts. + */ + @VisibleForTesting + @ThreadCompatible + public NestedSet<Artifact> discoverInputsFromDotdFiles( + Path execRoot, ArtifactResolver artifactResolver, Reply reply) + throws ActionExecutionException { + NestedSetBuilder<Artifact> inputs = NestedSetBuilder.stableOrder(); + if (getDotdFile() == null) { + return inputs.build(); + } + try { + // Read .d file. + DependencySet depSet = processDepset(execRoot, reply); + + // Determine prefixes of allowed absolute inclusions. + CppConfiguration toolchain = cppConfiguration; + List<Path> systemIncludePrefixes = new ArrayList<>(); + for (PathFragment includePath : toolchain.getBuiltInIncludeDirectories()) { + if (includePath.isAbsolute()) { + systemIncludePrefixes.add(execRoot.getFileSystem().getPath(includePath)); + } + } + + // Check inclusions. + IncludeProblems problems = new IncludeProblems(); + Map<PathFragment, Artifact> allowedDerivedInputsMap = getAllowedDerivedInputsMap(); + for (Path execPath : depSet.getDependencies()) { + // Module .pcm files are generated and thus aren't declared inputs. + if (execPath.getBaseName().endsWith(".pcm")) { + continue; + } + PathFragment execPathFragment = execPath.asFragment(); + if (execPathFragment.isAbsolute()) { + // Absolute includes from system paths are ignored. + if (FileSystemUtils.startsWithAny(execPath, systemIncludePrefixes)) { + continue; + } + // Since gcc is given only relative paths on the command line, + // non-system include paths here should never be absolute. If they + // are, it's probably due to a non-hermetic #include, & we should stop + // the build with an error. + if (execPath.startsWith(execRoot)) { + execPathFragment = execPath.relativeTo(execRoot); // funky but tolerable path + } else { + problems.add(execPathFragment.getPathString()); + continue; + } + } + Artifact artifact = allowedDerivedInputsMap.get(execPathFragment); + if (artifact == null) { + artifact = artifactResolver.resolveSourceArtifact(execPathFragment); + } + if (artifact != null) { + inputs.add(artifact); + // In some cases, execution backends need extra files for each included file. Add them + // to the set of actual inputs. + inputs.addAll(specialInputsHandler.getInputsForIncludedFile(artifact, artifactResolver)); + } else { + // Abort if we see files that we can't resolve, likely caused by + // undeclared includes or illegal include constructs. + problems.add(execPathFragment.getPathString()); + } + } + //TODO(b/22551695): Remove in favor of seperate implementations. + if (semantics == null || 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); + } + return inputs.build(); + } + @Override public Iterable<Artifact> resolveInputsFromCache( ArtifactResolver artifactResolver, @@ -960,7 +1059,7 @@ public class CppCompileAction extends AbstractAction } } - protected Map<PathFragment, Artifact> getAllowedDerivedInputsMap() { + private Map<PathFragment, Artifact> getAllowedDerivedInputsMap() { Map<PathFragment, Artifact> allowedDerivedInputMap = new HashMap<>(); addToMap(allowedDerivedInputMap, mandatoryInputs); addToMap(allowedDerivedInputMap, getDeclaredIncludeSrcs()); @@ -1083,10 +1182,9 @@ public class CppCompileAction extends AbstractAction // This is the .d file scanning part. IncludeScanningContext scanningContext = executor.getContext(IncludeScanningContext.class); - Path execRoot = executor.getExecRoot(); - NestedSet<Artifact> discoveredInputs = - discoverInputsFromDotdFiles(execRoot, scanningContext.getArtifactResolver(), reply); + discoverInputsFromDotdFiles( + executor.getExecRoot(), scanningContext.getArtifactResolver(), reply); reply = null; // Clear in-memory .d files early. // Post-execute "include scanning", which modifies the action inputs to match what the compile @@ -1104,67 +1202,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()); - } - - @VisibleForTesting - public NestedSet<Artifact> discoverInputsFromDotdFiles( - Path execRoot, ArtifactResolver artifactResolver, Reply reply) - throws ActionExecutionException { - if (getDotdFile() == null) { - return NestedSetBuilder.<Artifact>stableOrder().build(); - } - HeaderDiscovery.Builder discoveryBuilder = new HeaderDiscovery.Builder() - .setAction(this) - .setDotdFile(getDotdFile()) - .setSourceFile(getSourceFile()) - .setSpecialInputsHandler(specialInputsHandler) - .setDependencySet(processDepset(execRoot, reply)) - .setPermittedSystemIncludePrefixes(getPermittedSystemIncludePrefixes(execRoot)) - .setAllowedDerivedinputsMap(getAllowedDerivedInputsMap()); - - if (cppSemantics.needsIncludeValidation()) { - discoveryBuilder.shouldValidateInclusions(); - } - - return discoveryBuilder - .build() - .discoverInputsFromDotdFiles(execRoot, artifactResolver); - } - - public DependencySet processDepset(Path execRoot, Reply reply) throws ActionExecutionException { - try { - DotdFile dotdFile = getDotdFile(); - Preconditions.checkNotNull(dotdFile); - DependencySet depSet = new DependencySet(execRoot); - // artifact() is null if we are using in-memory .d files. We also want to prepare for the - // case where we expected an in-memory .d file, but we did not get an appropriate response. - // Perhaps we produced the file locally. - if (dotdFile.artifact() != null || reply == null) { - return depSet.read(dotdFile.getPath()); - } else { - // This is an in-memory .d file. - return depSet.process(reply.getContents()); - } - } 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); - } - } - - public List<Path> getPermittedSystemIncludePrefixes(Path execRoot) { - CppConfiguration toolchain = cppConfiguration; - List<Path> systemIncludePrefixes = new ArrayList<>(); - for (PathFragment includePath : toolchain.getBuiltInIncludeDirectories()) { - if (includePath.isAbsolute()) { - systemIncludePrefixes.add(execRoot.getFileSystem().getPath(includePath)); - } + // 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 == null || semantics.needsIncludeValidation()) { + validateInclusions( + discoveredInputs, + actionExecutionContext.getArtifactExpander(), + executor.getEventHandler()); } - return systemIncludePrefixes; } /** 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 c0fd32769c..95bac0bed5 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 @@ -314,6 +314,7 @@ public class CppCompileActionBuilder { sourceFile, shouldScanIncludes, shouldPruneModules, + usePic, sourceLabel, realMandatoryInputs, outputFile, 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 9eae435422..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 @@ -494,8 +494,6 @@ public final class CppModel { CppCompileActionBuilder builder = initializeCompileAction(moduleMapArtifact, moduleMapLabel, /*forInterface=*/ true); - builder.setSemantics(semantics); - // A header module compile action is just like a normal compile action, but: // - the compiled source file is the module map // - it creates a header module (.pcm file). 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 97fe0825fe..631e697331 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 @@ -34,7 +34,6 @@ import com.google.devtools.build.lib.analysis.RuleContext; import com.google.devtools.build.lib.analysis.config.BuildConfiguration; import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.collect.nestedset.NestedSet; -import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder; import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadCompatible; import com.google.devtools.build.lib.events.Event; import com.google.devtools.build.lib.rules.cpp.CcToolchainFeatures.FeatureConfiguration; @@ -42,7 +41,6 @@ import com.google.devtools.build.lib.util.Preconditions; 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.Path; import com.google.devtools.build.lib.vfs.PathFragment; import java.io.IOException; import java.util.UUID; @@ -81,7 +79,7 @@ public class FakeCppCompileAction extends CppCompileAction { ImmutableList<String> copts, Predicate<String> nocopts, RuleContext ruleContext, - CppSemantics cppSemantics) { + CppSemantics semantics) { super( owner, features, @@ -90,6 +88,7 @@ public class FakeCppCompileAction extends CppCompileAction { sourceFile, shouldScanIncludes, shouldPruneModules, + usePic, sourceLabel, mandatoryInputs, outputFile, @@ -117,7 +116,7 @@ public class FakeCppCompileAction extends CppCompileAction { ImmutableMap.<String, String>of(), CppCompileAction.CPP_COMPILE, ruleContext, - cppSemantics); + semantics); this.tempOutputFile = Preconditions.checkNotNull(tempOutputFile); } @@ -148,30 +147,9 @@ public class FakeCppCompileAction extends CppCompileAction { } } IncludeScanningContext scanningContext = executor.getContext(IncludeScanningContext.class); - Path execRoot = executor.getExecRoot(); - - NestedSet<Artifact> discoveredInputs; - if (getDotdFile() == null) { - discoveredInputs = NestedSetBuilder.<Artifact>stableOrder().build(); - } else { - HeaderDiscovery.Builder discoveryBuilder = new HeaderDiscovery.Builder() - .setAction(this) - .setDotdFile(getDotdFile()) - .setSourceFile(getSourceFile()) - .setSpecialInputsHandler(specialInputsHandler) - .setDependencySet(processDepset(execRoot, reply)) - .setPermittedSystemIncludePrefixes(getPermittedSystemIncludePrefixes(execRoot)) - .setAllowedDerivedinputsMap(getAllowedDerivedInputsMap()); - - if (cppSemantics.needsIncludeValidation()) { - discoveryBuilder.shouldValidateInclusions(); - } - - discoveredInputs = discoveryBuilder - .build() - .discoverInputsFromDotdFiles(execRoot, scanningContext.getArtifactResolver()); - } - + NestedSet<Artifact> discoveredInputs = + discoverInputsFromDotdFiles( + executor.getExecRoot(), scanningContext.getArtifactResolver(), reply); reply = null; // Clear in-memory .d files early. // Even cc_fake_binary rules need to properly declare their dependencies... diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/HeaderDiscovery.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/HeaderDiscovery.java deleted file mode 100644 index 96c7b816b5..0000000000 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/HeaderDiscovery.java +++ /dev/null @@ -1,216 +0,0 @@ -// Copyright 2016 The Bazel Authors. All rights reserved. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package com.google.devtools.build.lib.rules.cpp; - -import com.google.common.annotations.VisibleForTesting; -import com.google.common.base.Preconditions; -import com.google.devtools.build.lib.actions.Action; -import com.google.devtools.build.lib.actions.ActionExecutionException; -import com.google.devtools.build.lib.actions.Artifact; -import com.google.devtools.build.lib.actions.ArtifactResolver; -import com.google.devtools.build.lib.collect.nestedset.NestedSet; -import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder; -import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadCompatible; -import com.google.devtools.build.lib.rules.cpp.CppCompileAction.DotdFile; -import com.google.devtools.build.lib.rules.cpp.CppCompileAction.SpecialInputsHandler; -import com.google.devtools.build.lib.util.DependencySet; -import com.google.devtools.build.lib.vfs.FileSystemUtils; -import com.google.devtools.build.lib.vfs.Path; -import com.google.devtools.build.lib.vfs.PathFragment; -import java.util.List; -import java.util.Map; - -/** Manages the process of obtaining inputs used in a compilation from .d files. */ -public class HeaderDiscovery { - - private final Action action; - private final Artifact sourceFile; - private final DotdFile dotdFile; - - private final SpecialInputsHandler specialInputsHandler; - private final boolean shouldValidateInclusions; - - private final DependencySet depSet; - private final List<Path> permittedSystemIncludePrefixes; - private final Map<PathFragment, Artifact> allowedDerivedInputsMap; - - /** - * Creates a HeaderDiscover instance - * - * @param action the action instance requiring header discovery - * @param sourceFile the source file for the compile - * @param dotdFile the .d file used for header discovery - * @param specialInputsHandler the SpecialInputsHandler for the build - * @param shouldValidateInclusions true if include validation should be performed - */ - public HeaderDiscovery( - Action action, - Artifact sourceFile, - DotdFile dotdFile, - SpecialInputsHandler specialInputsHandler, - boolean shouldValidateInclusions, - DependencySet depSet, - List<Path> permittedSystemIncludePrefixes, - Map<PathFragment, Artifact> allowedDerivedInputsMap) { - this.action = Preconditions.checkNotNull(action); - this.sourceFile = Preconditions.checkNotNull(sourceFile); - this.dotdFile = Preconditions.checkNotNull(dotdFile); - this.specialInputsHandler = specialInputsHandler; - this.shouldValidateInclusions = shouldValidateInclusions; - this.depSet = depSet; - this.permittedSystemIncludePrefixes = permittedSystemIncludePrefixes; - this.allowedDerivedInputsMap = allowedDerivedInputsMap; - } - - /** - * Returns a collection with additional input artifacts relevant to the action by reading the - * dynamically-discovered dependency information from the .d file after the action has run. - * - * <p>Artifacts are considered inputs but not "mandatory" inputs. - * - * @throws ActionExecutionException iff the .d is missing (when required), malformed, or has - * unresolvable included artifacts. - */ - @VisibleForTesting - @ThreadCompatible - public NestedSet<Artifact> discoverInputsFromDotdFiles( - Path execRoot, ArtifactResolver artifactResolver) throws ActionExecutionException { - NestedSetBuilder<Artifact> inputs = NestedSetBuilder.stableOrder(); - if (dotdFile == null) { - return inputs.build(); - } - List<Path> systemIncludePrefixes = permittedSystemIncludePrefixes; - - // Check inclusions. - IncludeProblems problems = new IncludeProblems(); - for (Path execPath : depSet.getDependencies()) { - // Module .pcm files are generated and thus aren't declared inputs. - if (execPath.getBaseName().endsWith(".pcm")) { - continue; - } - PathFragment execPathFragment = execPath.asFragment(); - if (execPathFragment.isAbsolute()) { - // Absolute includes from system paths are ignored. - if (FileSystemUtils.startsWithAny(execPath, systemIncludePrefixes)) { - continue; - } - // Since gcc is given only relative paths on the command line, - // non-system include paths here should never be absolute. If they - // are, it's probably due to a non-hermetic #include, & we should stop - // the build with an error. - if (execPath.startsWith(execRoot)) { - execPathFragment = execPath.relativeTo(execRoot); // funky but tolerable path - } else { - problems.add(execPathFragment.getPathString()); - continue; - } - } - Artifact artifact = allowedDerivedInputsMap.get(execPathFragment); - if (artifact == null) { - artifact = artifactResolver.resolveSourceArtifact(execPathFragment); - } - if (artifact != null) { - inputs.add(artifact); - // In some cases, execution backends need extra files for each included file. Add them - // to the set of actual inputs. - if (specialInputsHandler != null) { - inputs.addAll(specialInputsHandler.getInputsForIncludedFile(artifact, artifactResolver)); - } - } else { - // Abort if we see files that we can't resolve, likely caused by - // undeclared includes or illegal include constructs. - problems.add(execPathFragment.getPathString()); - } - } - if (shouldValidateInclusions) { - problems.assertProblemFree(action, sourceFile); - } - return inputs.build(); - } - - /** A Builder for HeaderDiscovery */ - public static class Builder { - private Action action; - private Artifact sourceFile; - private DotdFile dotdFile; - private SpecialInputsHandler specialInputsHandler; - private boolean shouldValidateInclusions = false; - - private DependencySet depSet; - private List<Path> permittedSystemIncludePrefixes; - private Map<PathFragment, Artifact> allowedDerivedInputsMap; - - /** Sets the action for which to discover inputs. */ - public Builder setAction(Action action) { - this.action = action; - return this; - } - - /** Sets the source file for which to discover inputs. */ - public Builder setSourceFile(Artifact sourceFile) { - this.sourceFile = sourceFile; - return this; - } - - /** Sets the dotd file to be used to discover inputs. */ - public Builder setDotdFile(DotdFile dotdFile) { - this.dotdFile = dotdFile; - return this; - } - - /** Sets the SpecialInputsHandler for inputs to this build. */ - public Builder setSpecialInputsHandler(SpecialInputsHandler specialInputsHandler) { - this.specialInputsHandler = specialInputsHandler; - return this; - } - - /** Sets that this compile should validate inclusions against the dotd file. */ - public Builder shouldValidateInclusions() { - this.shouldValidateInclusions = true; - return this; - } - - /** Sets the DependencySet capturing used headers by this compile. */ - public Builder setDependencySet(DependencySet depSet) { - this.depSet = depSet; - return this; - } - - /** Sets prefixes of allowed absolute inclusions */ - public Builder setPermittedSystemIncludePrefixes(List<Path> systemIncludePrefixes) { - this.permittedSystemIncludePrefixes = systemIncludePrefixes; - return this; - } - - /** Sets permitted inputs to the build */ - public Builder setAllowedDerivedinputsMap(Map<PathFragment, Artifact> allowedDerivedInputsMap) { - this.allowedDerivedInputsMap = allowedDerivedInputsMap; - return this; - } - - /** Creates a CppHeaderDiscovery instance. */ - public HeaderDiscovery build() { - return new HeaderDiscovery( - action, - sourceFile, - dotdFile, - specialInputsHandler, - shouldValidateInclusions, - depSet, - permittedSystemIncludePrefixes, - allowedDerivedInputsMap); - } - } -} diff --git a/src/main/java/com/google/devtools/build/lib/rules/objc/CompilationSupport.java b/src/main/java/com/google/devtools/build/lib/rules/objc/CompilationSupport.java index 1e45cd5d52..3a5a370491 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/objc/CompilationSupport.java +++ b/src/main/java/com/google/devtools/build/lib/rules/objc/CompilationSupport.java @@ -83,7 +83,6 @@ import com.google.devtools.build.lib.rules.apple.AppleConfiguration; import com.google.devtools.build.lib.rules.apple.AppleToolchain; import com.google.devtools.build.lib.rules.apple.Platform; import com.google.devtools.build.lib.rules.apple.Platform.PlatformType; -import com.google.devtools.build.lib.rules.cpp.CppCompileAction.DotdFile; import com.google.devtools.build.lib.rules.cpp.CppModuleMap; import com.google.devtools.build.lib.rules.cpp.CppModuleMapAction; import com.google.devtools.build.lib.rules.objc.XcodeProvider.Builder; @@ -656,21 +655,19 @@ public final class CompilationSupport { boolean runCodeCoverage = buildConfiguration.isCodeCoverageEnabled() && ObjcRuleClasses.isInstrumentable(sourceFile); boolean hasSwiftSources = compilationArtifacts.hasSwiftSources(); - DotdFile dotdFile = intermediateArtifacts.dotdFile(sourceFile); - CustomCommandLine commandLine = - compileActionCommandLine( - sourceFile, - objFile, - objcProvider, - priorityHeaders, - moduleMap, - compilationArtifacts.getPchFile(), - Optional.of(dotdFile.artifact()), - otherFlags, - runCodeCoverage, - isCPlusPlusSource, - hasSwiftSources); + CustomCommandLine commandLine = compileActionCommandLine( + sourceFile, + objFile, + objcProvider, + priorityHeaders, + moduleMap, + compilationArtifacts.getPchFile(), + Optional.of(intermediateArtifacts.dotdFile(sourceFile)), + otherFlags, + runCodeCoverage, + isCPlusPlusSource, + hasSwiftSources); Optional<Artifact> gcnoFile = Optional.absent(); if (runCodeCoverage && !buildConfiguration.isLLVMCoverageMapFormatEnabled()) { @@ -687,25 +684,24 @@ public final class CompilationSupport { moduleMapInputs = objcProvider.get(MODULE_MAP); } - // TODO(bazel-team): Remove private headers from inputs once they're added to the provider. + // TODO(bazel-team): Remote private headers from inputs once they're added to the provider. ruleContext.registerAction( - ObjcCompileAction.Builder.createObjcCompileActionBuilderWithAppleEnv( + ObjcRuleClasses.spawnAppleEnvActionBuilder( appleConfiguration, appleConfiguration.getSingleArchPlatform()) - .setSourceFile(sourceFile) - .addMandatoryInputs(swiftHeader.asSet()) - .addTransitiveMandatoryInputs(moduleMapInputs) - .addTransitiveMandatoryInputs(objcProvider.get(STATIC_FRAMEWORK_FILE)) - .addTransitiveMandatoryInputs(objcProvider.get(DYNAMIC_FRAMEWORK_FILE)) - .setDotdFile(dotdFile) - .addInputs(compilationArtifacts.getPrivateHdrs()) - .addInputs(compilationArtifacts.getPchFile().asSet()) .setMnemonic("ObjcCompile") .setExecutable(xcrunwrapper(ruleContext)) .setCommandLine(commandLine) + .addInput(sourceFile) + .addInputs(swiftHeader.asSet()) + .addTransitiveInputs(moduleMapInputs) .addOutput(objFile) .addOutputs(gcnoFile.asSet()) - .addOutput(dotdFile.artifact()) + .addOutput(intermediateArtifacts.dotdFile(sourceFile)) .addTransitiveInputs(objcProvider.get(HEADER)) + .addInputs(compilationArtifacts.getPrivateHdrs()) + .addTransitiveInputs(objcProvider.get(STATIC_FRAMEWORK_FILE)) + .addTransitiveInputs(objcProvider.get(DYNAMIC_FRAMEWORK_FILE)) + .addInputs(compilationArtifacts.getPchFile().asSet()) .build(ruleContext)); } diff --git a/src/main/java/com/google/devtools/build/lib/rules/objc/IntermediateArtifacts.java b/src/main/java/com/google/devtools/build/lib/rules/objc/IntermediateArtifacts.java index 811e698ff7..9b8ced2dfd 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/objc/IntermediateArtifacts.java +++ b/src/main/java/com/google/devtools/build/lib/rules/objc/IntermediateArtifacts.java @@ -20,7 +20,6 @@ import com.google.devtools.build.lib.analysis.RuleConfiguredTarget.Mode; import com.google.devtools.build.lib.analysis.RuleContext; import com.google.devtools.build.lib.analysis.config.BuildConfiguration; import com.google.devtools.build.lib.cmdline.Label; -import com.google.devtools.build.lib.rules.cpp.CppCompileAction.DotdFile; import com.google.devtools.build.lib.rules.cpp.CppModuleMap; import com.google.devtools.build.lib.util.Preconditions; import com.google.devtools.build.lib.vfs.FileSystemUtils; @@ -398,9 +397,11 @@ public final class IntermediateArtifacts { return appendExtension("_runner.sh"); } - /** Dependency file that is generated when compiling the {@code source} artifact. */ - public DotdFile dotdFile(Artifact source) { - return new DotdFile(inUniqueObjsDir(source, ".d")); + /** + * Dependency file that is generated when compiling the {@code source} artifact. + */ + public Artifact dotdFile(Artifact source) { + return inUniqueObjsDir(source, ".d"); } /** diff --git a/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcCompileAction.java b/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcCompileAction.java deleted file mode 100644 index 0f5fee51b3..0000000000 --- a/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcCompileAction.java +++ /dev/null @@ -1,304 +0,0 @@ -// Copyright 2016 The Bazel Authors. All rights reserved. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package com.google.devtools.build.lib.rules.objc; - -import static com.google.devtools.build.lib.collect.nestedset.Order.STABLE_ORDER; - -import com.google.common.annotations.VisibleForTesting; -import com.google.common.base.Preconditions; -import com.google.common.collect.ImmutableList; -import com.google.common.collect.ImmutableMap; -import com.google.common.collect.ImmutableSet; -import com.google.devtools.build.lib.actions.ActionExecutionContext; -import com.google.devtools.build.lib.actions.ActionExecutionException; -import com.google.devtools.build.lib.actions.ActionOwner; -import com.google.devtools.build.lib.actions.Artifact; -import com.google.devtools.build.lib.actions.ArtifactResolver; -import com.google.devtools.build.lib.actions.Executor; -import com.google.devtools.build.lib.actions.ResourceSet; -import com.google.devtools.build.lib.analysis.actions.CommandLine; -import com.google.devtools.build.lib.analysis.actions.SpawnAction; -import com.google.devtools.build.lib.collect.nestedset.NestedSet; -import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder; -import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadCompatible; -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.CppCompileAction.DotdFile; -import com.google.devtools.build.lib.rules.cpp.HeaderDiscovery; -import com.google.devtools.build.lib.rules.cpp.IncludeScanningContext; -import com.google.devtools.build.lib.util.DependencySet; -import com.google.devtools.build.lib.util.Fingerprint; -import com.google.devtools.build.lib.vfs.Path; -import com.google.devtools.build.lib.vfs.PathFragment; -import java.io.IOException; -import java.util.HashMap; -import java.util.Map; - -/** - * An action that compiles objc or objc++ source. - * - * <p>We don't use a plain SpawnAction here because we implement .d input pruning, which requires - * post-execution filtering of input artifacts. - * - * <p>We don't use a CppCompileAction because the ObjcCompileAction uses custom logic instead of the - * CROSSTOOL to construct its command line. - */ -public class ObjcCompileAction extends SpawnAction { - - private final DotdFile dotdFile; - private final Artifact sourceFile; - private final NestedSet<Artifact> mandatoryInputs; - - - private static final String GUID = "a00d5bac-a72c-4f0f-99a7-d5fdc6072137"; - - private ObjcCompileAction( - ActionOwner owner, - Iterable<Artifact> tools, - Iterable<Artifact> inputs, - Iterable<Artifact> outputs, - ResourceSet resourceSet, - CommandLine argv, - ImmutableMap<String, String> environment, - ImmutableMap<String, String> executionInfo, - String progressMessage, - ImmutableMap<PathFragment, Artifact> inputManifests, - String mnemonic, - boolean executeUnconditionally, - ExtraActionInfoSupplier<?> extraActionInfoSupplier, - DotdFile dotdFile, - Artifact sourceFile, - NestedSet<Artifact> mandatoryInputs) { - super( - owner, - tools, - inputs, - outputs, - resourceSet, - argv, - environment, - ImmutableSet.<String>of(), - executionInfo, - progressMessage, - inputManifests, - mnemonic, - executeUnconditionally, - extraActionInfoSupplier); - - this.dotdFile = dotdFile; - this.sourceFile = sourceFile; - this.mandatoryInputs = mandatoryInputs; - } - - @Override - public boolean discoversInputs() { - return true; - } - - @Override - public Iterable<Artifact> discoverInputs(ActionExecutionContext actionExecutionContext) { - // We do not use include scanning for objc - return getInputs(); - } - - @Override - public void execute(ActionExecutionContext actionExecutionContext) - throws ActionExecutionException, InterruptedException { - super.execute(actionExecutionContext); - - Executor executor = actionExecutionContext.getExecutor(); - IncludeScanningContext scanningContext = executor.getContext(IncludeScanningContext.class); - NestedSet<Artifact> discoveredInputs = - discoverInputsFromDotdFiles(executor.getExecRoot(), scanningContext.getArtifactResolver()); - - updateActionInputs(discoveredInputs); - } - - @VisibleForTesting - public NestedSet<Artifact> discoverInputsFromDotdFiles( - Path execRoot, ArtifactResolver artifactResolver) throws ActionExecutionException { - if (dotdFile == null) { - return NestedSetBuilder.<Artifact>stableOrder().build(); - } - return new HeaderDiscovery.Builder() - .setAction(this) - .setSourceFile(sourceFile) - .setDotdFile(dotdFile) - .setDependencySet(processDepset(execRoot)) - .setPermittedSystemIncludePrefixes(ImmutableList.<Path>of()) - .setAllowedDerivedinputsMap(getAllowedDerivedInputsMap()) - .build() - .discoverInputsFromDotdFiles(execRoot, artifactResolver); - } - - private DependencySet processDepset(Path execRoot) throws ActionExecutionException { - try { - DependencySet depSet = new DependencySet(execRoot); - return depSet.read(dotdFile.getPath()); - } 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); - } - } - - /** Utility function that adds artifacts to an input map, but only if they are sources. */ - private void addToMapIfSource(Map<PathFragment, Artifact> map, Iterable<Artifact> artifacts) { - for (Artifact artifact : artifacts) { - if (!artifact.isSourceArtifact()) { - map.put(artifact.getExecPath(), artifact); - } - } - } - - private Map<PathFragment, Artifact> getAllowedDerivedInputsMap() { - Map<PathFragment, Artifact> allowedDerivedInputMap = new HashMap<>(); - addToMapIfSource(allowedDerivedInputMap, getInputs()); - allowedDerivedInputMap.put(sourceFile.getExecPath(), sourceFile); - return allowedDerivedInputMap; - } - - /** - * Recalculates this action's live input collection, including sources, middlemen. - * - * @throws ActionExecutionException iff any errors happen during update. - */ - @VisibleForTesting - @ThreadCompatible - public final synchronized void updateActionInputs(NestedSet<Artifact> discoveredInputs) - throws ActionExecutionException { - NestedSetBuilder<Artifact> inputs = NestedSetBuilder.stableOrder(); - Profiler.instance().startTask(ProfilerTask.ACTION_UPDATE, this); - try { - inputs.addTransitive(mandatoryInputs); - inputs.addTransitive(discoveredInputs); - } finally { - Profiler.instance().completeTask(ProfilerTask.ACTION_UPDATE); - setInputs(inputs.build()); - } - } - - @Override - public String computeKey() { - Fingerprint f = new Fingerprint(); - f.addString(GUID); - f.addString(super.computeKey()); - f.addBoolean(dotdFile.artifact() == null); - f.addPath(dotdFile.getSafeExecPath()); - return f.hexDigestAndReset(); - } - - /** A Builder for ObjcCompileAction */ - public static class Builder extends SpawnAction.Builder { - - private DotdFile dotdFile; - private Artifact sourceFile; - private final NestedSetBuilder<Artifact> mandatoryInputs = new NestedSetBuilder<>(STABLE_ORDER); - - /** - * Creates a new compile action builder with apple environment variables set that are typically - * needed by the apple toolchain. - */ - public static ObjcCompileAction.Builder createObjcCompileActionBuilderWithAppleEnv( - AppleConfiguration appleConfiguration, Platform targetPlatform) { - return (Builder) - new ObjcCompileAction.Builder() - .setExecutionInfo(ObjcRuleClasses.darwinActionExecutionRequirement()) - .setEnvironment( - ObjcRuleClasses.appleToolchainEnvironment(appleConfiguration, targetPlatform)); - } - - @Override - public Builder addTools(Iterable<Artifact> artifacts) { - super.addTools(artifacts); - mandatoryInputs.addAll(artifacts); - return this; - } - - /** Sets a .d file that will used to prune input headers */ - public Builder setDotdFile(DotdFile dotdFile) { - Preconditions.checkNotNull(dotdFile); - this.dotdFile = dotdFile; - return this; - } - - /** Sets the source file that is being compiled in this action */ - public Builder setSourceFile(Artifact sourceFile) { - Preconditions.checkNotNull(sourceFile); - this.sourceFile = sourceFile; - this.mandatoryInputs.add(sourceFile); - this.addInput(sourceFile); - return this; - } - - /** Add an input that cannot be pruned */ - public Builder addMandatoryInput(Artifact input) { - Preconditions.checkNotNull(input); - this.mandatoryInputs.add(input); - this.addInput(input); - return this; - } - - /** Add inputs that cannot be pruned */ - public Builder addMandatoryInputs(Iterable<Artifact> input) { - Preconditions.checkNotNull(input); - this.mandatoryInputs.addAll(input); - this.addInputs(input); - return this; - } - - /** Add inputs that cannot be pruned */ - public Builder addTransitiveMandatoryInputs(NestedSet<Artifact> input) { - Preconditions.checkNotNull(input); - this.mandatoryInputs.addTransitive(input); - this.addTransitiveInputs(input); - return this; - } - - @Override - protected SpawnAction createSpawnAction( - ActionOwner owner, - NestedSet<Artifact> tools, - NestedSet<Artifact> inputsAndTools, - ImmutableList<Artifact> outputs, - ResourceSet resourceSet, - CommandLine actualCommandLine, - ImmutableMap<String, String> env, - ImmutableSet<String> clientEnvironmentVariables, - ImmutableMap<String, String> executionInfo, - String progressMessage, - ImmutableMap<PathFragment, Artifact> inputAndToolManifests, - String mnemonic) { - return new ObjcCompileAction( - owner, - tools, - inputsAndTools, - outputs, - resourceSet, - actualCommandLine, - env, - executionInfo, - progressMessage, - inputAndToolManifests, - mnemonic, - executeUnconditionally, - extraActionInfoSupplier, - dotdFile, - sourceFile, - mandatoryInputs.build()); - } - } -} |