Dig out the builtin include file from the filegroup of libc_top instead of special-casing it in CppConfiguration.
This seems to be the most reasonable solution. I was toying with the idea of adding a field to CROSSTOOL but that would fail if you set libc_top to something other than what was specified in that file. If I had a infinite amount of time, I'd create a custom rule called cc_libc where libc_top would point so that this file can be referenced by an attribute, but since I don't, this seems to be workable compromise.
Also note that contrary to what you'd glean from the code, we don't actually have "compile" and "link" filegroups for libc.
--
MOS_MIGRATED_REVID=118921101
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java b/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java
index 12a2445..7a4f72a 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java
@@ -32,7 +32,6 @@
import com.google.devtools.build.lib.actions.Artifact;
import com.google.devtools.build.lib.actions.ArtifactFactory;
import com.google.devtools.build.lib.actions.ArtifactOwner;
-import com.google.devtools.build.lib.actions.PackageRootResolver;
import com.google.devtools.build.lib.actions.Root;
import com.google.devtools.build.lib.analysis.ExtraActionArtifactsProvider.ExtraArtifactSet;
import com.google.devtools.build.lib.analysis.config.BinTools;
@@ -404,10 +403,10 @@
});
}
- private void prepareToBuild(BuildConfigurationCollection configurations,
- PackageRootResolver resolver) throws ViewCreationFailedException {
+ private void prepareToBuild(BuildConfigurationCollection configurations)
+ throws ViewCreationFailedException {
for (BuildConfiguration config : configurations.getAllConfigurations()) {
- config.prepareToBuild(directories.getExecRoot(), getArtifactFactory(), resolver);
+ config.prepareToBuild(directories.getExecRoot());
}
}
@@ -485,7 +484,7 @@
}
}
- prepareToBuild(configurations, new SkyframePackageRootResolver(skyframeExecutor, eventHandler));
+ prepareToBuild(configurations);
skyframeExecutor.injectWorkspaceStatusData();
SkyframeAnalysisResult skyframeAnalysisResult;
try {
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java b/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java
index 49740ec..6032d10 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java
@@ -29,8 +29,6 @@
import com.google.common.collect.Maps;
import com.google.common.collect.Multimap;
import com.google.common.collect.MutableClassToInstanceMap;
-import com.google.devtools.build.lib.actions.ArtifactFactory;
-import com.google.devtools.build.lib.actions.PackageRootResolver;
import com.google.devtools.build.lib.actions.Root;
import com.google.devtools.build.lib.analysis.BlazeDirectories;
import com.google.devtools.build.lib.analysis.ConfiguredRuleClassProvider;
@@ -179,8 +177,7 @@
* <p>Do not use this method to change your fragment's state.
*/
@SuppressWarnings("unused")
- public void prepareHook(Path execPath, ArtifactFactory artifactFactory,
- PackageRootResolver resolver) throws ViewCreationFailedException {
+ public void prepareHook(Path execPath) throws ViewCreationFailedException {
}
/**
@@ -2341,10 +2338,9 @@
* <p>C++ also requires this to resolve artifacts that are unconditionally included in every
* compilation.</p>
*/
- public void prepareToBuild(Path execRoot, ArtifactFactory artifactFactory,
- PackageRootResolver resolver) throws ViewCreationFailedException {
+ public void prepareToBuild(Path execRoot) throws ViewCreationFailedException {
for (Fragment fragment : fragments.values()) {
- fragment.prepareHook(execRoot, artifactFactory, resolver);
+ fragment.prepareHook(execRoot);
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchain.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchain.java
index feefa5a..6cc3a5c 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchain.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchain.java
@@ -15,6 +15,7 @@
import static com.google.devtools.build.lib.syntax.Type.BOOLEAN;
+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.Actions;
@@ -48,6 +49,11 @@
* Implementation for the cc_toolchain rule.
*/
public class CcToolchain implements RuleConfiguredTargetFactory {
+ /**
+ * This file (found under the sysroot) may be unconditionally included in every C/C++ compilation.
+ */
+ private static final PathFragment BUILTIN_INCLUDE_FILE_SUFFIX =
+ new PathFragment("include/stdc-predef.h");
@Override
public ConfiguredTarget create(RuleContext ruleContext) {
@@ -60,7 +66,7 @@
final NestedSet<Artifact> objcopy = getFiles(ruleContext, "objcopy_files");
final NestedSet<Artifact> link = getFiles(ruleContext, "linker_files");
final NestedSet<Artifact> dwp = getFiles(ruleContext, "dwp_files");
- final NestedSet<Artifact> libcLink = inputsForLibcLink(ruleContext);
+ final NestedSet<Artifact> libcLink = inputsForLibc(ruleContext);
String purposePrefix = Actions.escapeLabel(label) + "_";
String runtimeSolibDirBase = "_solib_" + "_" + Actions.escapeLabel(label);
final PathFragment runtimeSolibDir = ruleContext.getConfiguration()
@@ -143,7 +149,6 @@
boolean supportsParamFiles = ruleContext.attributes().get("supports_param_files", BOOLEAN);
boolean supportsHeaderParsing =
ruleContext.attributes().get("supports_header_parsing", BOOLEAN);
-
CcToolchainProvider provider =
new CcToolchainProvider(
Preconditions.checkNotNull(ruleContext.getFragment(CppConfiguration.class)),
@@ -163,7 +168,8 @@
context,
supportsParamFiles,
supportsHeaderParsing,
- getBuildVariables(ruleContext));
+ getBuildVariables(ruleContext),
+ getBuiltinIncludes(ruleContext));
RuleConfiguredTargetBuilder builder =
new RuleConfiguredTargetBuilder(ruleContext)
.add(CcToolchainProvider.class, provider)
@@ -192,10 +198,21 @@
return builder.build();
}
- private NestedSet<Artifact> inputsForLibcLink(RuleContext ruleContext) {
- TransitiveInfoCollection libcLink = ruleContext.getPrerequisite(":libc_link", Mode.HOST);
- return libcLink != null
- ? libcLink.getProvider(FileProvider.class).getFilesToBuild()
+ private ImmutableList<Artifact> getBuiltinIncludes(RuleContext ruleContext) {
+ ImmutableList.Builder<Artifact> result = ImmutableList.builder();
+ for (Artifact artifact : inputsForLibc(ruleContext)) {
+ if (artifact.getExecPath().endsWith(BUILTIN_INCLUDE_FILE_SUFFIX)) {
+ result.add(artifact);
+ }
+ }
+
+ return result.build();
+ }
+
+ private NestedSet<Artifact> inputsForLibc(RuleContext ruleContext) {
+ TransitiveInfoCollection libc = ruleContext.getPrerequisite(":libc_top", Mode.HOST);
+ return libc != null
+ ? libc.getProvider(FileProvider.class).getFilesToBuild()
: NestedSetBuilder.<Artifact>emptySet(Order.STABLE_ORDER);
}
@@ -203,17 +220,14 @@
NestedSet<Artifact> crosstoolMiddleman) {
return NestedSetBuilder.<Artifact>stableOrder()
.addTransitive(crosstoolMiddleman)
- // Use "libc_link" here, because it is functionally identical to the case
- // below. If we introduce separate filegroups for compiling and linking, we
- // need to fix that here.
- .addTransitive(AnalysisUtils.getMiddlemanFor(ruleContext, ":libc_link"))
+ .addTransitive(AnalysisUtils.getMiddlemanFor(ruleContext, ":libc_top"))
.build();
}
private NestedSet<Artifact> fullInputsForLink(RuleContext ruleContext, NestedSet<Artifact> link) {
return NestedSetBuilder.<Artifact>stableOrder()
.addTransitive(link)
- .addTransitive(AnalysisUtils.getMiddlemanFor(ruleContext, ":libc_link"))
+ .addTransitive(AnalysisUtils.getMiddlemanFor(ruleContext, ":libc_top"))
.add(ruleContext.getAnalysisEnvironment().getEmbeddedToolArtifact(
CppRuleClasses.BUILD_INTERFACE_SO))
.build();
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 9673e04..05f866f 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
@@ -13,6 +13,7 @@
// limitations under the License.
package com.google.devtools.build.lib.rules.cpp;
+import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.devtools.build.lib.actions.Artifact;
import com.google.devtools.build.lib.analysis.TransitiveInfoProvider;
@@ -55,7 +56,8 @@
CppCompilationContext.EMPTY,
false,
false,
- ImmutableMap.<String, String>of());
+ ImmutableMap.<String, String>of(),
+ ImmutableList.<Artifact>of());
@Nullable private final CppConfiguration cppConfiguration;
private final NestedSet<Artifact> crosstool;
@@ -75,6 +77,7 @@
private final boolean supportsParamFiles;
private final boolean supportsHeaderParsing;
private final Map<String, String> buildVariables;
+ private final ImmutableList<Artifact> builtinIncludeFiles;
public CcToolchainProvider(
@Nullable CppConfiguration cppConfiguration,
@@ -94,7 +97,8 @@
CppCompilationContext cppCompilationContext,
boolean supportsParamFiles,
boolean supportsHeaderParsing,
- Map<String, String> buildVariables) {
+ Map<String, String> buildVariables,
+ ImmutableList<Artifact> builtinIncludeFiles) {
this.cppConfiguration = cppConfiguration;
this.crosstool = Preconditions.checkNotNull(crosstool);
this.crosstoolMiddleman = Preconditions.checkNotNull(crosstoolMiddleman);
@@ -113,6 +117,7 @@
this.supportsParamFiles = supportsParamFiles;
this.supportsHeaderParsing = supportsHeaderParsing;
this.buildVariables = buildVariables;
+ this.builtinIncludeFiles = builtinIncludeFiles;
}
/**
@@ -243,4 +248,12 @@
public Map<String, String> getBuildVariables() {
return buildVariables;
}
+
+ /**
+ * Return the set of include files that may be included even if they are not mentioned in the
+ * source file or any of the headers included by it.
+ */
+ public ImmutableList<Artifact> getBuiltinIncludeFiles() {
+ return builtinIncludeFiles;
+ }
}
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainRule.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainRule.java
index 7ab982f..4519103 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainRule.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainRule.java
@@ -47,7 +47,7 @@
return ruleClass.endsWith("cc_toolchain");
}
- private static final LateBoundLabel<BuildConfiguration> LIBC_LINK =
+ private static final LateBoundLabel<BuildConfiguration> LIBC_TOP =
new LateBoundLabel<BuildConfiguration>(CppConfiguration.class) {
@Override
public Label getDefault(
@@ -75,7 +75,7 @@
.add(attr("supports_param_files", BOOLEAN).value(true))
.add(attr("supports_header_parsing", BOOLEAN).value(false))
// TODO(bazel-team): Should be using the TARGET configuration.
- .add(attr(":libc_link", LABEL).cfg(HOST).value(LIBC_LINK))
+ .add(attr(":libc_top", LABEL).cfg(HOST).value(LIBC_TOP))
.build();
}
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 3b7681b..abbd13a 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
@@ -153,6 +153,7 @@
private final CppCompilationContext context;
private final Collection<PathFragment> extraSystemIncludePrefixes;
private final Iterable<IncludeScannable> lipoScannables;
+ private final ImmutableList<Artifact> builtinIncludeFiles;
private final CppCompileCommandLine cppCompileCommandLine;
private final boolean usePic;
@@ -276,6 +277,7 @@
// We do not need to include the middleman artifact since it is a generated
// artifact and will definitely exist prior to this action execution.
this.mandatoryInputs = mandatoryInputs;
+ this.builtinIncludeFiles = CppHelper.getToolchain(ruleContext).getBuiltinIncludeFiles();
verifyIncludePaths(ruleContext);
}
@@ -342,8 +344,8 @@
@Nullable
@Override
- public Artifact getBuiltInIncludeFile() {
- return cppConfiguration.getBuiltInIncludeFile();
+ public List<Artifact> getBuiltInIncludeFiles() {
+ return builtinIncludeFiles;
}
public String getHostSystemName() {
@@ -1106,6 +1108,10 @@
f.addPaths(getDeclaredIncludeSrcsInStableOrder());
f.addPaths(getExtraSystemIncludePrefixes());
f.addPaths(Artifact.asSortedPathFragments(getMandatoryInputs()));
+ // I'm not sure if getBuiltInIncludeFiles() is necessary here, since before an action cache hit
+ // is reported, include scanning needs to be run, and thus the changed set of files would be
+ // noticed. Better be safe than sorry.
+ f.addPaths(Artifact.asSortedPathFragments(getBuiltInIncludeFiles()));
return f.hexDigestAndReset();
}
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 e3be6f5..f151dea 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,10 +25,6 @@
import com.google.common.collect.ListMultimap;
import com.google.common.collect.Maps;
import com.google.common.collect.Multimap;
-import com.google.devtools.build.lib.actions.Artifact;
-import com.google.devtools.build.lib.actions.ArtifactFactory;
-import com.google.devtools.build.lib.actions.PackageRootResolutionException;
-import com.google.devtools.build.lib.actions.PackageRootResolver;
import com.google.devtools.build.lib.actions.Root;
import com.google.devtools.build.lib.analysis.RuleContext;
import com.google.devtools.build.lib.analysis.ViewCreationFailedException;
@@ -70,8 +66,6 @@
import java.util.Set;
import java.util.zip.ZipException;
-import javax.annotation.Nullable;
-
/**
* This class represents the C/C++ parts of the {@link BuildConfiguration},
* including the host architecture, target architecture, compiler version, and
@@ -203,12 +197,6 @@
public static final String FDO_STAMP_MACRO = "BUILD_FDO_TYPE";
/**
- * This file (found under the sysroot) may be unconditionally included in every C/C++ compilation.
- */
- private static final PathFragment BUILT_IN_INCLUDE_PATH_FRAGMENT =
- new PathFragment("include/stdc-predef.h");
-
- /**
* Represents an optional flag that can be toggled using the package features mechanism.
*/
@Immutable
@@ -308,7 +296,6 @@
private final PathFragment sysroot;
private final PathFragment runtimeSysroot;
private final List<PathFragment> builtInIncludeDirectories;
- private Artifact builtInIncludeFile;
private final Map<String, PathFragment> toolPaths;
private final PathFragment ldExecutable;
@@ -1202,15 +1189,6 @@
}
/**
- * Returns the built-in header automatically included by the toolchain compiler. All C++ files
- * may implicitly include this file. May be null if {@link #getSysroot} is null.
- */
- @Nullable
- public Artifact getBuiltInIncludeFile() {
- return builtInIncludeFile;
- }
-
- /**
* Returns the sysroot to be used. If the toolchain compiler does not support
* different sysroots, or the sysroot is the same as the default sysroot, then
* this method returns <code>null</code>.
@@ -1914,28 +1892,7 @@
}
@Override
- public void prepareHook(Path execRoot, ArtifactFactory artifactFactory,
- PackageRootResolver resolver) throws ViewCreationFailedException {
- // TODO(bazel-team): Remove the "relative" guard. sysroot should always be relative, and this
- // should be enforced in the creation of CppConfiguration.
- if (getSysroot() != null && !getSysroot().isAbsolute()) {
- Root sysrootRoot;
- try {
- sysrootRoot = Iterables.getOnlyElement(
- resolver.findPackageRootsForFiles(
- // See doc of findPackageRootsForFiles for why we need a getChild here.
- ImmutableList.of(getSysroot().getChild("dummy_child"))).entrySet()).getValue();
- } catch (PackageRootResolutionException prre) {
- throw new ViewCreationFailedException("Failed to determine sysroot", prre);
- }
-
- PathFragment sysrootExecPath = sysroot.getRelative(BUILT_IN_INCLUDE_PATH_FRAGMENT);
- if (sysrootRoot.getPath().getRelative(sysrootExecPath).exists()) {
- builtInIncludeFile = Preconditions.checkNotNull(
- artifactFactory.getSourceArtifact(sysrootExecPath, sysrootRoot),
- "%s %s", sysrootRoot, sysroot);
- }
- }
+ public void prepareHook(Path execRoot) throws ViewCreationFailedException {
try {
getFdoSupport().prepareToBuild(execRoot);
} catch (ZipException e) {
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/IncludeScannable.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/IncludeScannable.java
index 88b142b..267a750 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/IncludeScannable.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/IncludeScannable.java
@@ -73,7 +73,7 @@
* does not mention it.
*/
@Nullable
- Artifact getBuiltInIncludeFile();
+ List<Artifact> getBuiltInIncludeFiles();
/**
* Returns the artifact relative to which the {@code getCmdlineIncludes()} should be interpreted.
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/IncludeScanner.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/IncludeScanner.java
index 0e14c4b..c359284 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/IncludeScanner.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/IncludeScanner.java
@@ -106,10 +106,7 @@
Set<Artifact> includes = Sets.newConcurrentHashSet();
final List<PathFragment> absoluteBuiltInIncludeDirs = new ArrayList<>();
- Artifact builtInInclude = action.getBuiltInIncludeFile();
- if (builtInInclude != null) {
- includes.add(builtInInclude);
- }
+ includes.addAll(action.getBuiltInIncludeFiles());
Profiler profiler = Profiler.instance();
try {
