Skip to content

Commit

Permalink
Remove unused package-loading related code from CppConfiguration
Browse files Browse the repository at this point in the history
Keeping RedirectChaser there so this is change stays user-invisible. Redirect
chaser ignores visibility, so by removing it builds that used to work can stop
working (if the cc_toolchain should not be visible from cc_toolchain_suite, or
if cc_toolchain was an alias that pointed to somewhere else, and that something
else was not visible).

Observable effect of this cl is that error messages in cases when CROSSTOOL is missing or when --crosstool_top doesn't point to cc_toolchain_suite change. They are moved to the analysis phase, and their wording changed. I expect and hope that this is not going to break anybody.

RELNOTES: None.
PiperOrigin-RevId: 222813249
  • Loading branch information
hlopko authored and Copybara-Service committed Nov 26, 2018
1 parent 0aa5412 commit 3aa8079
Show file tree
Hide file tree
Showing 15 changed files with 82 additions and 417 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
import com.google.devtools.build.lib.packages.RuleClass;
import com.google.devtools.build.lib.rules.cpp.CcImportRule;
import com.google.devtools.build.lib.rules.cpp.CcToolchain;
import com.google.devtools.build.lib.rules.cpp.CcToolchainProvider;
import com.google.devtools.build.lib.rules.cpp.CppConfiguration;
import com.google.devtools.build.lib.rules.cpp.CppRuleClasses;

Expand All @@ -35,6 +36,7 @@ public RuleClass build(RuleClass.Builder builder, RuleDefinitionEnvironment env)
.requiresConfigurationFragments(CppConfiguration.class)
.add(
attr(CcToolchain.CC_TOOLCHAIN_DEFAULT_ATTRIBUTE_NAME, LABEL)
.mandatoryProviders(CcToolchainProvider.PROVIDER.id())
.value(CppRuleClasses.ccToolchainAttribute(env)))
.add(
attr(CcToolchain.CC_TOOLCHAIN_TYPE_ATTRIBUTE_NAME, NODEP_LABEL)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@
import com.google.devtools.build.lib.packages.TriState;
import com.google.devtools.build.lib.rules.cpp.CcInfo;
import com.google.devtools.build.lib.rules.cpp.CcToolchain;
import com.google.devtools.build.lib.rules.cpp.CcToolchainProvider;
import com.google.devtools.build.lib.rules.cpp.CppFileTypes;
import com.google.devtools.build.lib.rules.cpp.CppRuleClasses;
import com.google.devtools.build.lib.rules.cpp.CppRuleClasses.CcIncludeScanningRule;
Expand Down Expand Up @@ -100,6 +101,7 @@ public RuleClass build(RuleClass.Builder builder, RuleDefinitionEnvironment env)
return builder
.add(
attr(CcToolchain.CC_TOOLCHAIN_DEFAULT_ATTRIBUTE_NAME, LABEL)
.mandatoryProviders(CcToolchainProvider.PROVIDER.id())
.value(CppRuleClasses.ccToolchainAttribute(env)))
.add(
attr(CcToolchain.CC_TOOLCHAIN_TYPE_ATTRIBUTE_NAME, NODEP_LABEL)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import com.google.devtools.build.lib.analysis.config.HostTransition;
import com.google.devtools.build.lib.packages.RuleClass;
import com.google.devtools.build.lib.rules.cpp.CcToolchain;
import com.google.devtools.build.lib.rules.cpp.CcToolchainProvider;
import com.google.devtools.build.lib.rules.cpp.CppConfiguration;
import com.google.devtools.build.lib.rules.genrule.GenRuleBaseRule;
import com.google.devtools.build.lib.rules.java.JavaConfiguration;
Expand Down Expand Up @@ -54,6 +55,7 @@ public RuleClass build(RuleClass.Builder builder, RuleDefinitionEnvironment env)
.add(attr("stamp", BOOLEAN).value(false))
.add(
attr(CcToolchain.CC_TOOLCHAIN_DEFAULT_ATTRIBUTE_NAME, LABEL)
.mandatoryProviders(CcToolchainProvider.PROVIDER.id())
.value(GenRuleBaseRule.ccToolchainAttribute(env)))
.add(
attr(CcToolchain.CC_TOOLCHAIN_TYPE_ATTRIBUTE_NAME, NODEP_LABEL)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import com.google.devtools.build.lib.analysis.config.InvalidConfigurationException;
import com.google.devtools.build.lib.cmdline.PackageIdentifier;
import com.google.devtools.build.lib.skyframe.PackageLookupValue;
import com.google.devtools.build.lib.skyframe.SkyframeBuildView.CcCrosstoolException;
import com.google.devtools.build.lib.vfs.FileSystemUtils;
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.PathFragment;
Expand Down Expand Up @@ -93,6 +94,13 @@ public SkyValue compute(SkyKey skyKey, Environment env)

// 3. Parse the crosstool file the into CrosstoolRelease
Path crosstoolFile = crosstoolFileValue.realRootedPath().asPath();
if (!crosstoolFile.exists()) {
throw new CcSkyframeSupportException(
String.format(
"there is no CROSSTOOL file at %s, which is needed for this cc_toolchain",
crosstool.toString()),
key);
}
try (InputStream inputStream = crosstoolFile.getInputStream()) {
String crosstoolContent = new String(FileSystemUtils.readContentAsLatin1(inputStream));
crosstoolRelease =
Expand All @@ -116,8 +124,12 @@ public String extractTag(SkyKey skyKey) {
/** Exception encapsulating IOExceptions thrown in {@link CcSkyframeSupportFunction} */
public static class CcSkyframeSupportException extends SkyFunctionException {

public CcSkyframeSupportException(Exception cause, SkyKey childKey) {
super(cause, childKey);
public CcSkyframeSupportException(Exception cause, CcSkyframeSupportValue.Key key) {
super(cause, key);
}

public CcSkyframeSupportException(String message, CcSkyframeSupportValue.Key key) {
super(new CcCrosstoolException(message), key);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -399,8 +399,7 @@ static CcToolchainProvider getCcToolchainProvider(
(CcSkyframeSupportValue)
skyframeEnv.getValueOrThrow(ccSupportKey, CcSkyframeSupportException.class);
} catch (CcSkyframeSupportException e) {
ruleContext.throwWithRuleError(e.getMessage());
throw new IllegalStateException("Should not be reached");
throw ruleContext.throwWithRuleError(e.getMessage());
}
if (skyframeEnv.valuesMissing()) {
return null;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@
import com.google.devtools.build.lib.analysis.platform.ToolchainInfo;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.packages.BuildType;
import com.google.devtools.build.lib.packages.RuleClass.ConfiguredTargetFactory.RuleErrorException;
import com.google.devtools.build.lib.syntax.Type;
import com.google.devtools.build.lib.view.config.crosstool.CrosstoolConfig.CrosstoolRelease;
import java.util.Map;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
import com.google.common.base.Preconditions;
import com.google.common.base.Verify;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.devtools.build.lib.analysis.config.AutoCpuConverter;
import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
import com.google.devtools.build.lib.analysis.config.BuildConfiguration.Options;
Expand All @@ -26,48 +27,19 @@
import com.google.devtools.build.lib.analysis.config.PerLabelOptions;
import com.google.devtools.build.lib.analysis.skylark.annotations.SkylarkConfigurationField;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.cmdline.LabelSyntaxException;
import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
import com.google.devtools.build.lib.events.Event;
import com.google.devtools.build.lib.events.EventHandler;
import com.google.devtools.build.lib.rules.cpp.CppConfigurationLoader.CppConfigurationParameters;
import com.google.devtools.build.lib.skylarkbuildapi.cpp.CppConfigurationApi;
import com.google.devtools.build.lib.skylarkinterface.SkylarkCallable;
import com.google.devtools.build.lib.vfs.FileSystemUtils;
import com.google.devtools.build.lib.vfs.PathFragment;
import javax.annotation.Nullable;

/**
* This class represents the C/C++ parts of the {@link BuildConfiguration}, including the host
* architecture, target architecture, compiler version, and a standard library version. It has
* information about the tools locations and the flags required for compiling.
*
* <p>Before {@link CppConfiguration} is created, two things need to be done:
*
* <ol>
* <li>choosing a {@link CcToolchainRule} label from {@code toolchains} map attribute of {@link
* CcToolchainSuiteRule}.
* <li>selection of a {@link
* com.google.devtools.build.lib.view.config.crosstool.CrosstoolConfig.CToolchain} from the
* CROSSTOOL file.
* </ol>
*
* <p>The process goes as follows:
*
* <p>Check for existence of {@link CcToolchainSuiteRule}.toolchains[<cpu>|<compiler>], if
* --compiler is specified, otherwise check for {@link CcToolchainSuiteRule}.toolchains[<cpu>].
*
* <ul>
* <li>if a value is found, load the {@link CcToolchainRule} rule and look for the {@code
* toolchain_identifier} attribute.
* <li>
* <ul>
* <li>if the attribute exists, loop through all the {@code CToolchain}s in CROSSTOOL and
* select the one with the matching toolchain identifier.
* <li>otherwise fall back to selecting the CToolchain from CROSSTOOL by matching the --cpu
* and --compiler values.
* </ul>
* <li>If a value is not found, select the CToolchain from CROSSTOOL by matching the --cpu and
* --compiler values, and construct the key as follows: <toolchain.cpu>|<toolchain.compiler>.
* </ul>
* architecture, target architecture, compiler version, and a standard library version.
*/
@Immutable
public final class CppConfiguration extends BuildConfiguration.Fragment
Expand Down Expand Up @@ -191,10 +163,12 @@ public String toString() {
private final boolean stripBinaries;
private final CompilationMode compilationMode;

static CppConfiguration create(CppConfigurationParameters params) {
CppOptions cppOptions = params.cppOptions;
static CppConfiguration create(
CpuTransformer cpuTransformer, Label redirectChasedCrosstoolTop, BuildOptions options)
throws InvalidConfigurationException {
CppOptions cppOptions = options.get(CppOptions.class);

Options commonOptions = params.commonOptions;
Options commonOptions = options.get(Options.class);
CompilationMode compilationMode = commonOptions.compilationMode;

ImmutableList.Builder<String> linkoptsBuilder = ImmutableList.builder();
Expand All @@ -203,12 +177,32 @@ static CppConfiguration create(CppConfigurationParameters params) {
linkoptsBuilder.add("-Wl,--eh-frame-hdr");
}

PathFragment fdoPath = null;
Label fdoProfileLabel = null;
if (cppOptions.getFdoOptimize() != null) {
if (cppOptions.getFdoOptimize().startsWith("//")) {
try {
fdoProfileLabel = Label.parseAbsolute(cppOptions.getFdoOptimize(), ImmutableMap.of());
} catch (LabelSyntaxException e) {
throw new InvalidConfigurationException(e);
}
} else {
fdoPath = PathFragment.create(cppOptions.getFdoOptimize());
try {
// We don't check for file existence, but at least the filename should be well-formed.
FileSystemUtils.checkBaseName(fdoPath.getBaseName());
} catch (IllegalArgumentException e) {
throw new InvalidConfigurationException(e);
}
}
}

return new CppConfiguration(
params.transformedCpu,
params.crosstoolTop,
cpuTransformer.getTransformer().apply(commonOptions.cpu),
redirectChasedCrosstoolTop,
Preconditions.checkNotNull(commonOptions.cpu),
params.fdoPath,
params.fdoOptimizeLabel,
fdoPath,
fdoProfileLabel,
ImmutableList.copyOf(cppOptions.conlyoptList),
ImmutableList.copyOf(cppOptions.coptList),
ImmutableList.copyOf(cppOptions.cxxoptList),
Expand Down
Loading

0 comments on commit 3aa8079

Please sign in to comment.