diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelRulesModule.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelRulesModule.java index 226f1355fc152d..f613d8ed7447f1 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelRulesModule.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelRulesModule.java @@ -20,8 +20,8 @@ import com.google.devtools.build.lib.analysis.config.BuildOptions; import com.google.devtools.build.lib.bazel.rules.cpp.BazelCppRuleClasses; import com.google.devtools.build.lib.bazel.rules.sh.BazelShRuleClasses; +import com.google.devtools.build.lib.rules.cpp.CcSkyframeSupportFunction; import com.google.devtools.build.lib.rules.cpp.CcSkyframeSupportValue; -import com.google.devtools.build.lib.rules.cpp.CcSupportFunction; import com.google.devtools.build.lib.runtime.BlazeModule; import com.google.devtools.build.lib.runtime.BlazeRuntime; import com.google.devtools.build.lib.runtime.Command; @@ -99,7 +99,8 @@ public void initializeRuleClasses(ConfiguredRuleClassProvider.Builder builder) { @Override public void workspaceInit( BlazeRuntime runtime, BlazeDirectories directories, WorkspaceBuilder builder) { - builder.addSkyFunction(CcSkyframeSupportValue.SKYFUNCTION, new CcSupportFunction(directories)); + builder.addSkyFunction( + CcSkyframeSupportValue.SKYFUNCTION, new CcSkyframeSupportFunction(directories)); } @Override diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcSkyframeSupportFunction.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcSkyframeSupportFunction.java new file mode 100644 index 00000000000000..0e2192e4453b8e --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcSkyframeSupportFunction.java @@ -0,0 +1,114 @@ +// Copyright 2014 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.base.Preconditions; +import com.google.devtools.build.lib.actions.FileValue; +import com.google.devtools.build.lib.analysis.BlazeDirectories; +import com.google.devtools.build.lib.analysis.config.InvalidConfigurationException; +import com.google.devtools.build.lib.cmdline.Label; +import com.google.devtools.build.lib.vfs.FileSystemUtils; +import com.google.devtools.build.lib.vfs.Path; +import com.google.devtools.build.lib.vfs.Root; +import com.google.devtools.build.lib.vfs.RootedPath; +import com.google.devtools.build.lib.view.config.crosstool.CrosstoolConfig.CrosstoolRelease; +import com.google.devtools.build.skyframe.SkyFunction; +import com.google.devtools.build.skyframe.SkyFunctionException; +import com.google.devtools.build.skyframe.SkyKey; +import com.google.devtools.build.skyframe.SkyValue; +import java.io.IOException; +import java.io.InputStream; +import javax.annotation.Nullable; + +/** + * A {@link SkyFunction} that does things for FDO that a regular configured target is not allowed + * to. + * + *

This only exists because the value of {@code --fdo_optimize} can be a workspace-relative path + * and thus we need to depend on {@link BlazeDirectories} somehow, which neither the configuration + * nor the analysis phase can "officially" do. + * + *

The fix is probably to make it possible for {@link + * com.google.devtools.build.lib.analysis.actions.SymlinkAction} to create workspace-relative + * symlinks because action execution can hopefully depend on {@link BlazeDirectories}. + * + *

There is also the awful and incorrect {@link Path#exists()} call in {@link + * com.google.devtools.build.lib.view.proto.CcProtoProfileProvider#getProfile( + * com.google.devtools.build.lib.analysis.RuleContext)} which needs a {@link Path}. + */ +public class CcSkyframeSupportFunction implements SkyFunction { + private final BlazeDirectories directories; + + public CcSkyframeSupportFunction(BlazeDirectories directories) { + this.directories = Preconditions.checkNotNull(directories); + } + + @Nullable + @Override + public SkyValue compute(SkyKey skyKey, Environment env) + throws InterruptedException, CcSkyframeSupportException { + CcSkyframeSupportValue.Key key = (CcSkyframeSupportValue.Key) skyKey.argument(); + Path fdoZipPath = null; + CrosstoolRelease crosstoolRelease = null; + if (key.getFdoZipPath() != null) { + fdoZipPath = directories.getWorkspace().getRelative(key.getFdoZipPath()); + } + + if (key.getCrosstoolPath() != null) { + try { + Root root; + // Dear reader, if your eye just twitched and the thought cannot escape your mind that + // I should've used execroot, beware, execroot is created after the analysis, and this + // function is executed during the analysis. + if (key.getCrosstoolPath().startsWith(Label.EXTERNAL_PACKAGE_NAME)) { + root = Root.fromPath(directories.getOutputBase()); + } else { + root = Root.fromPath(directories.getWorkspace()); + } + FileValue crosstoolFileValue = + (FileValue) + env.getValue(FileValue.key(RootedPath.toRootedPath(root, key.getCrosstoolPath()))); + if (env.valuesMissing()) { + return null; + } + + Path crosstoolFile = crosstoolFileValue.realRootedPath().asPath(); + try (InputStream inputStream = crosstoolFile.getInputStream()) { + String crosstoolContent = new String(FileSystemUtils.readContentAsLatin1(inputStream)); + crosstoolRelease = + CrosstoolConfigurationLoader.toReleaseConfiguration( + "CROSSTOOL file " + key.getCrosstoolPath(), crosstoolContent); + } + } catch (IOException | InvalidConfigurationException e) { + throw new CcSkyframeSupportException(e, key); + } + } + + return new CcSkyframeSupportValue(fdoZipPath, crosstoolRelease); + } + + @Nullable + @Override + public String extractTag(SkyKey skyKey) { + return null; + } + + /** Exception encapsulating IOExceptions thrown in {@link CcSkyframeSupportFunction} */ + public static class CcSkyframeSupportException extends SkyFunctionException { + + public CcSkyframeSupportException(Exception cause, SkyKey childKey) { + super(cause, childKey); + } + } +} diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcSkyframeSupportValue.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcSkyframeSupportValue.java index 19aef127e3bfde..d7a6487e462490 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcSkyframeSupportValue.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcSkyframeSupportValue.java @@ -19,6 +19,7 @@ import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec; import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.PathFragment; +import com.google.devtools.build.lib.view.config.crosstool.CrosstoolConfig.CrosstoolRelease; import com.google.devtools.build.skyframe.SkyFunction; import com.google.devtools.build.skyframe.SkyFunctionName; import com.google.devtools.build.skyframe.SkyKey; @@ -28,9 +29,9 @@ /** * A container for the path to the FDO profile. * - *

{@link CcSkyframeSupportValue} is created from {@link CcSupportFunction} (a {@link + *

{@link CcSkyframeSupportValue} is created from {@link CcSkyframeSupportFunction} (a {@link * SkyFunction}), which is requested from Skyframe by the {@code cc_toolchain}/{@code - * cc_toolchain_suite} rule. It's done this way because the path depends on both a command line + * cc_toolchain_suite} rules. It's done this way because the path depends on both a command line * argument and the location of the workspace and the latter is not available either during * configuration creation or during the analysis phase. */ @@ -45,20 +46,26 @@ public class CcSkyframeSupportValue implements SkyValue { public static class Key implements SkyKey { private static final Interner interner = BlazeInterners.newWeakInterner(); - private final PathFragment filePath; + private final PathFragment fdoZipPath; + private final PathFragment crosstoolPath; - private Key(PathFragment filePath) { - this.filePath = filePath; + private Key(PathFragment fdoZipPath, PathFragment crosstoolPath) { + this.fdoZipPath = fdoZipPath; + this.crosstoolPath = crosstoolPath; } @AutoCodec.Instantiator @AutoCodec.VisibleForSerialization - static Key of(PathFragment filePath) { - return interner.intern(new Key(filePath)); + static Key of(PathFragment fdoZipPath, PathFragment crosstoolPath) { + return interner.intern(new Key(fdoZipPath, crosstoolPath)); } - public PathFragment getFilePath() { - return filePath; + public PathFragment getCrosstoolPath() { + return crosstoolPath; + } + + public PathFragment getFdoZipPath() { + return fdoZipPath; } @Override @@ -66,18 +73,18 @@ public boolean equals(Object o) { if (this == o) { return true; } - if (!(o instanceof Key)) { return false; } - - Key that = (Key) o; - return Objects.equals(this.filePath, that.filePath); + Key key = (Key) o; + return Objects.equals(fdoZipPath, key.fdoZipPath) + && Objects.equals(crosstoolPath, key.crosstoolPath); } @Override public int hashCode() { - return Objects.hash(filePath); + + return Objects.hash(fdoZipPath, crosstoolPath); } @Override @@ -88,19 +95,26 @@ public SkyFunctionName functionName() { /** Path of the profile file passed to {@code --fdo_optimize} */ // TODO(lberki): This should be a PathFragment. - // Except that CcProtoProfileProvider#getProfile() calls #exists() on it, which is ridiculously - // incorrect. - private final Path filePath; + // Except that CcProtoProfileProvider#getProfile() calls #exists() on it, + // This is all ridiculously incorrect and should be removed asap. + private final Path fdoZipPath; + + private final CrosstoolRelease crosstoolRelease; + + CcSkyframeSupportValue(Path fdoZipPath, CrosstoolRelease crosstoolRelease) { + this.fdoZipPath = fdoZipPath; + this.crosstoolRelease = crosstoolRelease; + } - CcSkyframeSupportValue(Path filePath) { - this.filePath = filePath; + public Path getFdoZipPath() { + return fdoZipPath; } - public Path getFilePath() { - return filePath; + public CrosstoolRelease getCrosstoolRelease() { + return crosstoolRelease; } - public static SkyKey key(PathFragment fdoProfileArgument) { - return Key.of(fdoProfileArgument); + public static SkyKey key(PathFragment fdoZipPath, PathFragment crosstoolPath) { + return Key.of(fdoZipPath, crosstoolPath); } } diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcSupportFunction.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcSupportFunction.java deleted file mode 100644 index 113361fe58da19..00000000000000 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcSupportFunction.java +++ /dev/null @@ -1,63 +0,0 @@ -// Copyright 2014 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.base.Preconditions; -import com.google.devtools.build.lib.analysis.BlazeDirectories; -import com.google.devtools.build.lib.vfs.Path; -import com.google.devtools.build.skyframe.SkyFunction; -import com.google.devtools.build.skyframe.SkyKey; -import com.google.devtools.build.skyframe.SkyValue; -import javax.annotation.Nullable; - -/** - * A {@link SkyFunction} that does things for FDO that a regular configured target is not allowed - * to. - * - *

This only exists because the value of {@code --fdo_optimize} can be a workspace-relative path - * and thus we need to depend on {@link BlazeDirectories} somehow, which neither the configuration - * nor the analysis phase can "officially" do. - * - *

The fix is probably to make it possible for {@link - * com.google.devtools.build.lib.analysis.actions.SymlinkAction} to create workspace-relative - * symlinks because action execution can hopefully depend on {@link BlazeDirectories}. - * - *

There is also the awful and incorrect {@link Path#exists()} call in {@link - * com.google.devtools.build.lib.view.proto.CcProtoProfileProvider#getProfile( - * com.google.devtools.build.lib.analysis.RuleContext)} which needs a {@link Path}. - */ -public class CcSupportFunction implements SkyFunction { - private final BlazeDirectories directories; - - public CcSupportFunction(BlazeDirectories directories) { - this.directories = Preconditions.checkNotNull(directories); - } - - @Nullable - @Override - public SkyValue compute(SkyKey skyKey, Environment env) throws InterruptedException { - CcSkyframeSupportValue.Key key = (CcSkyframeSupportValue.Key) skyKey.argument(); - Path resolvedPath = - key.getFilePath() == null - ? null - : directories.getWorkspace().getRelative(key.getFilePath()); - return new CcSkyframeSupportValue(resolvedPath); - } - - @Nullable - @Override - public String extractTag(SkyKey skyKey) { - return null; - } -} 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 3fc13b0613f74b..76b5b583eab634 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 @@ -14,6 +14,7 @@ package com.google.devtools.build.lib.rules.cpp; import static com.google.devtools.build.lib.syntax.Type.BOOLEAN; +import static com.google.devtools.build.lib.syntax.Type.STRING; import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableList; @@ -57,10 +58,12 @@ import com.google.devtools.build.lib.syntax.Type; import com.google.devtools.build.lib.util.FileType; import com.google.devtools.build.lib.util.Pair; +import com.google.devtools.build.lib.util.StringUtil; 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 com.google.devtools.build.lib.view.config.crosstool.CrosstoolConfig.CToolchain; +import com.google.devtools.build.lib.view.config.crosstool.CrosstoolConfig.CrosstoolRelease; import com.google.devtools.build.skyframe.SkyFunction; import com.google.devtools.build.skyframe.SkyKey; import com.google.protobuf.TextFormat; @@ -326,7 +329,6 @@ public ConfiguredTarget create(RuleContext ruleContext) BuildConfiguration configuration = Preconditions.checkNotNull(ruleContext.getConfiguration()); CppConfiguration cppConfiguration = Preconditions.checkNotNull(configuration.getFragment(CppConfiguration.class)); - CppToolchainInfo toolchainInfo = getCppToolchainInfo(ruleContext, cppConfiguration); PathFragment fdoZip = null; FdoInputFile prefetchHints = null; @@ -407,14 +409,28 @@ public ConfiguredTarget create(RuleContext ruleContext) return null; } - SkyKey fdoKey = CcSkyframeSupportValue.key(fdoZip); + // Is there a toolchain proto available on the target directly? + CToolchain toolchain = parseToolchainFromAttributes(ruleContext); + PathFragment crosstoolPath = null; + if (toolchain == null && cppConfiguration.getCrosstoolFromCcToolchainProtoAttribute() == null) { + crosstoolPath = + cppConfiguration + .getCrosstoolTopPathFragment() + .getChild(CrosstoolConfigurationLoader.CROSSTOOL_CONFIGURATION_FILENAME); + } + + SkyKey ccSupportKey = CcSkyframeSupportValue.key(fdoZip, crosstoolPath); SkyFunction.Environment skyframeEnv = ruleContext.getAnalysisEnvironment().getSkyframeEnv(); - CcSkyframeSupportValue fdoSupport = (CcSkyframeSupportValue) skyframeEnv.getValue(fdoKey); + CcSkyframeSupportValue ccSkyframeSupportValue = + (CcSkyframeSupportValue) skyframeEnv.getValue(ccSupportKey); if (skyframeEnv.valuesMissing()) { return null; } + CppToolchainInfo toolchainInfo = + getCppToolchainInfo(ruleContext, cppConfiguration, ccSkyframeSupportValue, toolchain); + final Label label = ruleContext.getLabel(); final NestedSet crosstool = ruleContext.getPrerequisite("all_files", Mode.HOST) .getProvider(FileProvider.class).getFilesToBuild(); @@ -550,12 +566,12 @@ public ConfiguredTarget create(RuleContext ruleContext) if (fdoMode == FdoMode.LLVM_FDO) { profileArtifact = convertLLVMRawProfileToIndexed( - fdoSupport.getFilePath().asFragment(), toolchainInfo, ruleContext); + ccSkyframeSupportValue.getFdoZipPath().asFragment(), toolchainInfo, ruleContext); if (ruleContext.hasErrors()) { return null; } } else if (fdoMode == FdoMode.AUTO_FDO || fdoMode == FdoMode.XBINARY_FDO) { - Path fdoProfile = fdoSupport.getFilePath(); + Path fdoProfile = ccSkyframeSupportValue.getFdoZipPath(); profileArtifact = ruleContext.getUniqueDirectoryArtifact( "fdo", fdoProfile.getBaseName(), @@ -608,7 +624,7 @@ public ConfiguredTarget create(RuleContext ruleContext) sysroot, fdoMode, new FdoProvider( - fdoSupport.getFilePath(), + ccSkyframeSupportValue.getFdoZipPath(), fdoMode, cppConfiguration.getFdoInstrument(), profileArtifact, @@ -653,7 +669,11 @@ public ConfiguredTarget create(RuleContext ruleContext) /** Finds an appropriate {@link CppToolchainInfo} for this target. */ private CppToolchainInfo getCppToolchainInfo( - RuleContext ruleContext, CppConfiguration cppConfiguration) throws RuleErrorException { + RuleContext ruleContext, + CppConfiguration cppConfiguration, + CcSkyframeSupportValue ccSkyframeSupportValue, + CToolchain toolchainFromCcToolchainAttribute) + throws RuleErrorException { if (cppConfiguration.enableCcToolchainConfigInfoFromSkylark()) { // Attempt to obtain CppToolchainInfo from the 'toolchain_config' attribute of cc_toolchain. @@ -676,7 +696,10 @@ private CppToolchainInfo getCppToolchainInfo( } // Attempt to find a toolchain based on the target attributes, not the configuration. - CToolchain toolchain = getToolchainFromAttributes(ruleContext, cppConfiguration); + CToolchain toolchain = toolchainFromCcToolchainAttribute; + if (toolchain == null) { + toolchain = getToolchainFromAttributes(ruleContext, cppConfiguration, ccSkyframeSupportValue); + } // If we found a toolchain, use it. try { @@ -699,15 +722,14 @@ private CppToolchainInfo getCppToolchainInfo( @Nullable private CToolchain parseToolchainFromAttributes(RuleContext ruleContext) throws RuleErrorException { - if (ruleContext.attributes().get("proto", Type.STRING).isEmpty()) { + String protoAttribute = StringUtil.emptyToNull(ruleContext.attributes().get("proto", STRING)); + if (protoAttribute == null) { return null; } - String data = ruleContext.attributes().get("proto", Type.STRING); - CToolchain.Builder builder = CToolchain.newBuilder(); try { - TextFormat.merge(data, builder); + TextFormat.merge(protoAttribute, builder); return builder.build(); } catch (ParseException e) { throw ruleContext.throwWithAttributeError("proto", "Could not parse CToolchain data"); @@ -753,25 +775,31 @@ private static ImmutableMap getToolchainForSkylark( @Nullable private CToolchain getToolchainFromAttributes( - RuleContext ruleContext, CppConfiguration cppConfiguration) throws RuleErrorException { - - // Is there a toolchain proto available on the target directly? - CToolchain toolchain = parseToolchainFromAttributes(ruleContext); - if (toolchain != null) { - return toolchain; - } + RuleContext ruleContext, + CppConfiguration cppConfiguration, + CcSkyframeSupportValue ccSkyframeSupportValue) + throws RuleErrorException { String toolchainIdentifier = ruleContext.attributes().get("toolchain_identifier", Type.STRING); String cpu = ruleContext.attributes().get("cpu", Type.STRING); String compiler = ruleContext.attributes().get("compiler", Type.STRING); try { + CrosstoolRelease crosstoolRelease; + if (cppConfiguration.getCrosstoolFromCcToolchainProtoAttribute() != null) { + // We have cc_toolchain_suite.proto attribute set, let's use it + crosstoolRelease = cppConfiguration.getCrosstoolFromCcToolchainProtoAttribute(); + } else { + // We use the proto from the CROSSTOOL file + crosstoolRelease = ccSkyframeSupportValue.getCrosstoolRelease(); + } + return CToolchainSelectionUtils.selectCToolchain( toolchainIdentifier, cpu, compiler, cppConfiguration.getTransformedCpuFromOptions(), cppConfiguration.getCompilerFromOptions(), - cppConfiguration.getCrosstoolFile().getProto()); + crosstoolRelease); } catch (InvalidConfigurationException e) { ruleContext.throwWithRuleError( String.format("Error while selecting cc_toolchain: %s", e.getMessage())); 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 0c9e75fe611ecd..d148191f3fa55e 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 @@ -34,13 +34,13 @@ import com.google.devtools.build.lib.events.EventHandler; import com.google.devtools.build.lib.events.Location; import com.google.devtools.build.lib.rules.cpp.CppConfigurationLoader.CppConfigurationParameters; -import com.google.devtools.build.lib.rules.cpp.CrosstoolConfigurationLoader.CrosstoolFile; import com.google.devtools.build.lib.rules.cpp.Link.LinkingMode; import com.google.devtools.build.lib.skylarkbuildapi.cpp.CppConfigurationApi; import com.google.devtools.build.lib.skylarkinterface.SkylarkCallable; import com.google.devtools.build.lib.syntax.EvalException; import com.google.devtools.build.lib.vfs.PathFragment; import com.google.devtools.build.lib.view.config.crosstool.CrosstoolConfig; +import com.google.devtools.build.lib.view.config.crosstool.CrosstoolConfig.CrosstoolRelease; import java.util.Map; import javax.annotation.Nullable; @@ -175,9 +175,16 @@ public String toString() { public static final String FDO_STAMP_MACRO = "BUILD_FDO_TYPE"; private final Label crosstoolTop; + /** + * cc_toolchain_suite allows to override CROSSTOOL by using proto attribute. This attribute value + * is stored here so cc_toolchain can access it in the analysis. Don't use this for anything, it + * will be removed when b/113849758 is fixed. If you do, I'll send bubo to take your keyboard + * away. + */ + @Deprecated private final CrosstoolRelease crosstoolFromCcToolchainProtoAttribute; + private final String transformedCpuFromOptions; private final String compilerFromOptions; - private final CrosstoolFile crosstoolFile; // TODO(lberki): desiredCpu *should* be always the same as targetCpu, except that we don't check // that the CPU we get from the toolchain matches BuildConfiguration.Options.cpu . So we store // it here so that the output directory doesn't depend on the CToolchain. When we will eventually @@ -264,9 +271,9 @@ static CppConfiguration create(CppConfigurationParameters params) return new CppConfiguration( params.crosstoolTop, + params.crosstoolFromCcToolchainProtoAttribute, params.transformedCpu, params.compiler, - params.crosstoolFile, Preconditions.checkNotNull(params.commonOptions.cpu), crosstoolTopPathFragment, params.fdoPath, @@ -301,9 +308,9 @@ static CppConfiguration create(CppConfigurationParameters params) private CppConfiguration( Label crosstoolTop, + CrosstoolRelease crosstoolFromCcToolchainProtoAttribute, String transformedCpuFromOptions, String compilerFromOptions, - CrosstoolFile crosstoolFile, String desiredCpu, PathFragment crosstoolTopPathFragment, PathFragment fdoPath, @@ -330,9 +337,9 @@ private CppConfiguration( boolean shouldProvideMakeVariables, CppToolchainInfo cppToolchainInfo) { this.crosstoolTop = crosstoolTop; + this.crosstoolFromCcToolchainProtoAttribute = crosstoolFromCcToolchainProtoAttribute; this.transformedCpuFromOptions = transformedCpuFromOptions; this.compilerFromOptions = compilerFromOptions; - this.crosstoolFile = crosstoolFile; this.desiredCpu = desiredCpu; this.crosstoolTopPathFragment = crosstoolTopPathFragment; this.fdoPath = fdoPath; @@ -390,11 +397,6 @@ public String getToolchainIdentifier() { return cppToolchainInfo.getToolchainIdentifier(); } - /** Returns the contents of the CROSSTOOL for this configuration. */ - public CrosstoolFile getCrosstoolFile() { - return crosstoolFile; - } - /** Returns the label of the CROSSTOOL for this configuration. */ public Label getCrosstoolTop() { return crosstoolTop; @@ -1180,6 +1182,17 @@ public boolean disableMakeVariables() { return cppOptions.disableMakeVariables || !cppOptions.enableMakeVariables; } + /** + * cc_toolchain_suite allows to override CROSSTOOL by using proto attribute. This attribute value + * is stored here so cc_toolchain can access it in the analysis. Don't use this for anything, it + * will be removed when b/113849758 is fixed. If you do, I'll send bubo to take your keyboard + * away. + */ + @Deprecated + public CrosstoolRelease getCrosstoolFromCcToolchainProtoAttribute() { + return crosstoolFromCcToolchainProtoAttribute; + } + public boolean enableLinkoptsInUserLinkFlags() { return cppOptions.enableLinkoptsInUserLinkFlags; } diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfigurationLoader.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfigurationLoader.java index d45a5a8f077fff..43790ea2039d0b 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfigurationLoader.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfigurationLoader.java @@ -14,6 +14,8 @@ package com.google.devtools.build.lib.rules.cpp; +import static com.google.devtools.build.lib.rules.cpp.CrosstoolConfigurationLoader.toReleaseConfiguration; + import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.devtools.build.lib.analysis.RedirectChaser; @@ -32,11 +34,12 @@ import com.google.devtools.build.lib.packages.NonconfigurableAttributeMapper; import com.google.devtools.build.lib.packages.Rule; import com.google.devtools.build.lib.packages.Target; -import com.google.devtools.build.lib.rules.cpp.CrosstoolConfigurationLoader.CrosstoolFile; import com.google.devtools.build.lib.syntax.Type; +import com.google.devtools.build.lib.util.StringUtil; import com.google.devtools.build.lib.vfs.FileSystemUtils; import com.google.devtools.build.lib.vfs.PathFragment; import com.google.devtools.build.lib.view.config.crosstool.CrosstoolConfig.CToolchain; +import com.google.devtools.build.lib.view.config.crosstool.CrosstoolConfig.CrosstoolRelease; import com.google.devtools.common.options.OptionsParsingException; import java.util.Map; import javax.annotation.Nullable; @@ -79,8 +82,6 @@ public CppConfiguration create(ConfigurationEnvironment env, BuildOptions option * Value class for all the data needed to create a {@link CppConfiguration}. */ public static class CppConfigurationParameters { - protected final CrosstoolConfigurationLoader.CrosstoolFile crosstoolFile; - protected final String cacheKeySuffix; protected final BuildConfiguration.Options commonOptions; protected final CppOptions cppOptions; protected final Label crosstoolTop; @@ -92,12 +93,12 @@ public static class CppConfigurationParameters { protected final CcToolchainConfigInfo ccToolchainConfigInfo; protected final String transformedCpu; protected final String compiler; + protected final CrosstoolRelease crosstoolFromCcToolchainProtoAttribute; CppConfigurationParameters( String transformedCpu, String compiler, - CrosstoolFile crosstoolFile, - String cacheKeySuffix, + CrosstoolRelease crosstoolFromCcToolchainProtoAttribute, BuildOptions buildOptions, PathFragment fdoPath, Label fdoOptimizeLabel, @@ -108,8 +109,7 @@ public static class CppConfigurationParameters { CcToolchainConfigInfo ccToolchainConfigInfo) { this.transformedCpu = transformedCpu; this.compiler = compiler; - this.crosstoolFile = crosstoolFile; - this.cacheKeySuffix = cacheKeySuffix; + this.crosstoolFromCcToolchainProtoAttribute = crosstoolFromCcToolchainProtoAttribute; this.commonOptions = buildOptions.get(BuildConfiguration.Options.class); this.cppOptions = buildOptions.get(CppOptions.class); this.fdoPath = fdoPath; @@ -149,9 +149,9 @@ protected CppConfigurationParameters createParameters( crosstoolTopLabel)); } - CrosstoolConfigurationLoader.CrosstoolFile file = + CrosstoolRelease crosstoolRelease = CrosstoolConfigurationLoader.readCrosstool(env, crosstoolTopLabel); - if (file == null) { + if (crosstoolRelease == null) { return null; } @@ -161,7 +161,12 @@ protected CppConfigurationParameters createParameters( transformedCpu + (cppOptions.cppCompiler == null ? "" : ("|" + cppOptions.cppCompiler)); Label ccToolchainLabel = selectCcToolchainLabel( - cppOptions, crosstoolTopLabel, (Rule) crosstoolTop, file, transformedCpu, key); + cppOptions, + crosstoolTopLabel, + (Rule) crosstoolTop, + crosstoolRelease, + transformedCpu, + key); Target ccToolchain = loadCcToolchainTarget(env, ccToolchainLabel); if (ccToolchain == null) { @@ -187,7 +192,7 @@ protected CppConfigurationParameters createParameters( compilerAttribute, transformedCpu, cppOptions.cppCompiler, - file.getProto()); + crosstoolRelease); cToolchain = CppToolchainInfo.addLegacyFeatures( @@ -204,6 +209,17 @@ protected CppConfigurationParameters createParameters( } } + String ccToolchainSuiteProtoAttributeValue = + StringUtil.emptyToNull( + NonconfigurableAttributeMapper.of((Rule) crosstoolTop).get("proto", Type.STRING)); + + CrosstoolRelease crosstoolFromCcToolchainProtoAttribute = null; + if (ccToolchainSuiteProtoAttributeValue != null) { + crosstoolFromCcToolchainProtoAttribute = + toReleaseConfiguration( + "cc_toolchain_suite rule " + crosstoolTopLabel, ccToolchainSuiteProtoAttributeValue); + } + PathFragment fdoPath = null; Label fdoProfileLabel = null; if (cppOptions.getFdoOptimize() != null) { @@ -227,8 +243,7 @@ protected CppConfigurationParameters createParameters( return new CppConfigurationParameters( transformedCpu, cppOptions.cppCompiler, - file, - file.getMd5(), + crosstoolFromCcToolchainProtoAttribute, options, fdoPath, fdoProfileLabel, @@ -263,7 +278,7 @@ private Label selectCcToolchainLabel( CppOptions cppOptions, Label crosstoolTopLabel, Rule crosstoolTop, - CrosstoolFile file, + CrosstoolRelease crosstoolRelease, String transformedCpu, String key) throws InvalidConfigurationException { @@ -293,7 +308,7 @@ private Label selectCcToolchainLabel( /* compilerAttribute= */ null, transformedCpu, compiler, - file.getProto()); + crosstoolRelease); ccToolchainLabel = toolchains.get(toolchain.getTargetCpu() + "|" + toolchain.getCompiler()); if (cppOptions.disableCcToolchainFromCrosstool) { throw new InvalidConfigurationException( diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CrosstoolConfigurationLoader.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CrosstoolConfigurationLoader.java index 693b1a5ffddfe0..370f6cdf2786e6 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CrosstoolConfigurationLoader.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CrosstoolConfigurationLoader.java @@ -14,8 +14,6 @@ package com.google.devtools.build.lib.rules.cpp; -import static java.nio.charset.StandardCharsets.UTF_8; - import com.google.common.annotations.VisibleForTesting; import com.google.common.cache.Cache; import com.google.common.cache.CacheBuilder; @@ -29,12 +27,9 @@ import com.google.devtools.build.lib.packages.Package; import com.google.devtools.build.lib.packages.Rule; import com.google.devtools.build.lib.packages.Target; -import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec; import com.google.devtools.build.lib.syntax.Type; -import com.google.devtools.build.lib.util.Fingerprint; import com.google.devtools.build.lib.vfs.FileSystemUtils; import com.google.devtools.build.lib.vfs.Path; -import com.google.devtools.build.lib.view.config.crosstool.CrosstoolConfig; import com.google.devtools.build.lib.view.config.crosstool.CrosstoolConfig.CrosstoolRelease; import com.google.protobuf.TextFormat; import com.google.protobuf.TextFormat.ParseException; @@ -53,7 +48,7 @@ * done in a saner way. */ public class CrosstoolConfigurationLoader { - private static final String CROSSTOOL_CONFIGURATION_FILENAME = "CROSSTOOL"; + static final String CROSSTOOL_CONFIGURATION_FILENAME = "CROSSTOOL"; /** * Cache for storing result of toReleaseConfiguration function based on path and md5 sum of @@ -61,110 +56,45 @@ public class CrosstoolConfigurationLoader { */ private static final Cache crosstoolReleaseCache = CacheBuilder.newBuilder().concurrencyLevel(4).maximumSize(100).build(); - /** A class that holds the results of reading a CROSSTOOL file. */ - @AutoCodec - public static class CrosstoolFile { - private final String location; - private final CrosstoolConfig.CrosstoolRelease proto; - private final String md5; - - @AutoCodec.Instantiator - CrosstoolFile(String location, CrosstoolConfig.CrosstoolRelease proto, String md5) { - this.location = location; - this.proto = proto; - this.md5 = md5; - } - - /** - * Returns a user-friendly location of the CROSSTOOL proto for error messages. - */ - public String getLocation() { - return location; - } - - /** - * Returns the parsed contents of the CROSSTOOL file. - */ - public CrosstoolConfig.CrosstoolRelease getProto() { - return proto; - } - - /** - * Returns an MD5 hash of the CROSSTOOL file contents. - */ - public String getMd5() { - return md5; - } - } private CrosstoolConfigurationLoader() { } /** - * Reads the given data String, which must be in ascii format, - * into a protocol buffer. It uses the name parameter for error - * messages. + * Reads the given data String, which must be in ascii format, into a protocol + * buffer. It uses the name parameter for error messages. * * @throws IOException if the parsing failed */ @VisibleForTesting - static CrosstoolConfig.CrosstoolRelease toReleaseConfiguration(String name, String data) - throws IOException { - CrosstoolConfig.CrosstoolRelease.Builder builder = - CrosstoolConfig.CrosstoolRelease.newBuilder(); + public static CrosstoolRelease toReleaseConfiguration(String name, String data) + throws InvalidConfigurationException { + CrosstoolRelease.Builder builder = CrosstoolRelease.newBuilder(); try { TextFormat.merge(data, builder); return builder.build(); } catch (ParseException e) { - throw new IOException("Could not read the crosstool configuration file '" + name + "', " - + "because of a parser error (" + e.getMessage() + ")"); + throw new InvalidConfigurationException( + "Could not read the crosstool configuration file '" + + name + + "', " + + "because of a parser error (" + + e.getMessage() + + ")"); } catch (UninitializedMessageException e) { - throw new IOException("Could not read the crosstool configuration file '" + name + "', " - + "because of an incomplete protocol buffer (" + e.getMessage() + ")"); + throw new InvalidConfigurationException( + "Could not read the crosstool configuration file '" + + name + + "', " + + "because of an incomplete protocol buffer (" + + e.getMessage() + + ")"); } } - /** - * This class is the in-memory representation of a text-formatted Crosstool proto file. - * - *

This layer of abstraction is here so that we can load these protos either from BUILD files - * or from CROSSTOOL files. - * - *

An implementation of this class should override {@link #getContents()} and call - * the constructor with the MD5 checksum of what that method will return and a human-readable name - * used in error messages. - */ - private abstract static class CrosstoolProto { - private final byte[] md5; - private final String name; - - private CrosstoolProto(byte[] md5, String name) { - this.md5 = md5; - this.name = name; - } - - /** - * The binary MD5 checksum of the proto. - */ - public byte[] getMd5() { - return md5; - } - - /** - * A user-friendly string describing the location of the proto. - */ - public String getName() { - return name; - } - - /** - * The proto itself. - */ - public abstract String getContents() throws IOException; - } - - private static CrosstoolProto getCrosstoolProtofromBuildFile( - ConfigurationEnvironment env, Label crosstoolTop) throws InterruptedException { + private static CrosstoolRelease getCrosstoolProtoFromBuildFile( + ConfigurationEnvironment env, Label crosstoolTop) + throws InterruptedException, InvalidConfigurationException { Target target; try { target = env.getTarget(crosstoolTop); @@ -183,22 +113,22 @@ private static CrosstoolProto getCrosstoolProtofromBuildFile( } final String contents = NonconfigurableAttributeMapper.of(rule).get("proto", Type.STRING); - byte[] md5 = new Fingerprint().addBytes(contents.getBytes(UTF_8)).digestAndReset(); - return new CrosstoolProto(md5, "cc_toolchain_suite rule " + crosstoolTop.toString()) { - - @Override - public String getContents() throws IOException { - return contents; - } - }; + return toReleaseConfiguration("cc_toolchain_suite rule " + crosstoolTop, contents); } - private static CrosstoolProto getCrosstoolProtoFromCrosstoolFile( + private static CrosstoolRelease findCrosstoolConfiguration( ConfigurationEnvironment env, Label crosstoolTop) throws IOException, InvalidConfigurationException, InterruptedException { - final Path path; + + CrosstoolRelease crosstoolProtoFromBuildFile = + getCrosstoolProtoFromBuildFile(env, crosstoolTop); + if (crosstoolProtoFromBuildFile != null) { + return crosstoolProtoFromBuildFile; + } + Path path; try { - Package containingPackage = env.getTarget(crosstoolTop).getPackage(); + Package containingPackage; + containingPackage = env.getTarget(crosstoolTop).getPackage(); if (containingPackage == null) { return null; } @@ -209,54 +139,40 @@ private static CrosstoolProto getCrosstoolProtoFromCrosstoolFile( } if (path == null || !path.exists()) { - return null; - } - - return new CrosstoolProto(path.getDigest(), "CROSSTOOL file " + path.getPathString()) { - @Override - public String getContents() throws IOException { - try (InputStream inputStream = path.getInputStream()) { - return new String(FileSystemUtils.readContentAsLatin1(inputStream)); - } - } - }; - } - - private static CrosstoolFile findCrosstoolConfiguration( - ConfigurationEnvironment env, Label crosstoolTop) - throws IOException, InvalidConfigurationException, InterruptedException { - - CrosstoolProto crosstoolProto = getCrosstoolProtofromBuildFile(env, crosstoolTop); - if (crosstoolProto == null) { - crosstoolProto = getCrosstoolProtoFromCrosstoolFile(env, crosstoolTop); - } - - if (crosstoolProto == null) { + // Normally you'd expect to return null when path is null (so Skyframe computes the value), + // and throw when path doesn't exist. But because {@link ConfigurationFragmentFunction} + // doesn't propagate the exceptions when there are valuesMissing(), we need to throw + // this exception always just to be sure it gets through. throw new InvalidConfigurationException("The crosstool_top you specified was resolved to '" + crosstoolTop + "', which does not contain a CROSSTOOL file."); - } else { - // Do this before we read the data, so if it changes, we get a different MD5 the next time. - // Alternatively, we could calculate the MD5 of the contents, which we also read, but this - // is faster if the file comes from a file system with md5 support. - final CrosstoolProto finalProto = crosstoolProto; - String md5 = BaseEncoding.base16().lowerCase().encode(finalProto.getMd5()); - CrosstoolConfig.CrosstoolRelease release; - try { - release = - crosstoolReleaseCache.get( - md5, () -> toReleaseConfiguration(finalProto.getName(), finalProto.getContents())); - } catch (ExecutionException e) { - throw new InvalidConfigurationException(e); - } + } - return new CrosstoolFile(finalProto.getName(), release, md5); + // Do this before we read the data, so if it changes, we get a different MD5 the next time. + // Alternatively, we could calculate the MD5 of the contents, which we also read, but this + // is faster if the file comes from a file system with md5 support. + String md5 = BaseEncoding.base16().lowerCase().encode(path.getDigest()); + CrosstoolRelease release; + try { + release = + crosstoolReleaseCache.get( + md5, + () -> { + try (InputStream inputStream = path.getInputStream()) { + return toReleaseConfiguration( + "CROSSTOOL file " + path, + new String(FileSystemUtils.readContentAsLatin1(inputStream))); + } + }); + } catch (ExecutionException e) { + throw new InvalidConfigurationException(e); } + + return release; } /** Reads a crosstool file. */ @Nullable - public static CrosstoolConfigurationLoader.CrosstoolFile readCrosstool( - ConfigurationEnvironment env, Label crosstoolTop) + public static CrosstoolRelease readCrosstool(ConfigurationEnvironment env, Label crosstoolTop) throws InvalidConfigurationException, InterruptedException { crosstoolTop = RedirectChaser.followRedirects(env, crosstoolTop, "crosstool_top"); if (crosstoolTop == null) { diff --git a/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisMock.java b/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisMock.java index 03368f63f71c2d..588e526a08aa03 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisMock.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisMock.java @@ -26,8 +26,8 @@ import com.google.devtools.build.lib.packages.util.MockCcSupport; import com.google.devtools.build.lib.packages.util.MockPythonSupport; import com.google.devtools.build.lib.packages.util.MockToolsConfig; +import com.google.devtools.build.lib.rules.cpp.CcSkyframeSupportFunction; import com.google.devtools.build.lib.rules.cpp.CcSkyframeSupportValue; -import com.google.devtools.build.lib.rules.cpp.CcSupportFunction; import com.google.devtools.build.lib.rules.repository.LocalRepositoryFunction; import com.google.devtools.build.lib.rules.repository.LocalRepositoryRule; import com.google.devtools.build.lib.rules.repository.RepositoryDelegatorFunction; @@ -136,7 +136,7 @@ repositoryHandlers, null, new AtomicBoolean(true), ImmutableMap::of, directories SkyFunctions.REPOSITORY, new RepositoryLoaderFunction(), CcSkyframeSupportValue.SKYFUNCTION, - new CcSupportFunction(directories)); + new CcSkyframeSupportFunction(directories)); } public static class Delegate extends AnalysisMock { diff --git a/src/test/java/com/google/devtools/build/lib/rules/cpp/CcToolchainTest.java b/src/test/java/com/google/devtools/build/lib/rules/cpp/CcToolchainTest.java index 843fc93cd75289..74bb8f54d5b31e 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/cpp/CcToolchainTest.java +++ b/src/test/java/com/google/devtools/build/lib/rules/cpp/CcToolchainTest.java @@ -105,6 +105,7 @@ public void testInterfaceSharedObjects() throws Exception { .setSupportsInterfaceSharedObjects(false) .buildPartial()); useConfiguration(); + invalidatePackages(); ConfiguredTarget target = getConfiguredTarget("//a:b"); CcToolchainProvider toolchainProvider = @@ -115,6 +116,7 @@ public void testInterfaceSharedObjects() throws Exception { .isFalse(); useConfiguration("--interface_shared_objects"); + invalidatePackages(); target = getConfiguredTarget("//a:b"); toolchainProvider = (CcToolchainProvider) target.get(ToolchainInfo.PROVIDER); assertThat( @@ -130,6 +132,7 @@ public void testInterfaceSharedObjects() throws Exception { .setSupportsInterfaceSharedObjects(true) .buildPartial()); useConfiguration(); + invalidatePackages(); target = getConfiguredTarget("//a:b"); toolchainProvider = (CcToolchainProvider) target.get(ToolchainInfo.PROVIDER); @@ -139,6 +142,7 @@ public void testInterfaceSharedObjects() throws Exception { .isTrue(); useConfiguration("--nointerface_shared_objects"); + invalidatePackages(); target = getConfiguredTarget("//a:b"); toolchainProvider = (CcToolchainProvider) target.get(ToolchainInfo.PROVIDER); assertThat( @@ -484,6 +488,7 @@ public void assertInvalidIncludeDirectoryMessage(String entry, String messageReg .buildPartial()); useConfiguration(); + invalidatePackages(); ConfiguredTarget target = getConfiguredTarget("//a:b"); CcToolchainProvider toolchainProvider = diff --git a/src/test/java/com/google/devtools/build/lib/rules/cpp/CrosstoolConfigurationLoaderTest.java b/src/test/java/com/google/devtools/build/lib/rules/cpp/CrosstoolConfigurationLoaderTest.java index de8db5bc949c74..90d833e01fbcf4 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/cpp/CrosstoolConfigurationLoaderTest.java +++ b/src/test/java/com/google/devtools/build/lib/rules/cpp/CrosstoolConfigurationLoaderTest.java @@ -785,7 +785,7 @@ public void testIncompleteFile() throws Exception { try { CrosstoolConfigurationLoader.toReleaseConfiguration("/CROSSTOOL", "major_version: \"12\""); fail(); - } catch (IOException e) { + } catch (InvalidConfigurationException e) { assertStringStartsWith( "Could not read the crosstool configuration file " + "'/CROSSTOOL', because of an incomplete protocol buffer", @@ -872,7 +872,7 @@ public void testInvalidFile() throws Exception { try { CrosstoolConfigurationLoader.toReleaseConfiguration("/CROSSTOOL", "some xxx : yak \""); fail(); - } catch (IOException e) { + } catch (InvalidConfigurationException e) { assertStringStartsWith( "Could not read the crosstool configuration file " + "'/CROSSTOOL', because of a parser error",