From b05bff63d4738a4db1641a13735864ed8e787d96 Mon Sep 17 00:00:00 2001 From: hlopko Date: Tue, 12 Sep 2017 10:51:44 +0200 Subject: Expose sysroot as a build variable This cl removes hardcoded --sysroot flag generation from bazel when constructing command line for C++ actions. The hardcoded flag is still exposed to Skylark (to stay backwards compatible). RELNOTES: None. PiperOrigin-RevId: 168346711 --- .../build/lib/rules/cpp/CcToolchainProvider.java | 11 ++- .../build/lib/rules/cpp/CompileCommandLine.java | 36 ++-------- .../build/lib/rules/cpp/CppActionConfigs.java | 78 +++++++++++++++------- .../build/lib/rules/cpp/CppCompileAction.java | 3 +- .../build/lib/rules/cpp/CppConfiguration.java | 53 ++++++++++----- .../build/lib/rules/cpp/CppLinkActionBuilder.java | 5 ++ .../devtools/build/lib/rules/cpp/CppModel.java | 10 ++- .../build/lib/rules/cpp/LinkCommandLine.java | 8 +-- 8 files changed, 120 insertions(+), 84 deletions(-) (limited to 'src/main/java') 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 6526877565..8f85034126 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 @@ -355,11 +355,11 @@ public final class CcToolchainProvider extends ToolchainInfo { + "rules. These should be appended to the command line after filtering." ) public ImmutableList getUnfilteredCompilerOptionsWithSysroot(Iterable features) { - return cppConfiguration.getUnfilteredCompilerOptions(features, sysroot); + return cppConfiguration.getUnfilteredCompilerOptionsDoNotUse(features, sysroot); } public ImmutableList getUnfilteredCompilerOptions(Iterable features) { - return cppConfiguration.getUnfilteredCompilerOptions(features, null); + return cppConfiguration.getUnfilteredCompilerOptionsDoNotUse(features, /* sysroot= */ null); } @SkylarkCallable( @@ -369,10 +369,15 @@ public final class CcToolchainProvider extends ToolchainInfo { "Returns the set of command-line linker options, including any flags " + "inferred from the command-line options." ) + public ImmutableList getLinkOptionsWithSysroot() { + return cppConfiguration.getLinkOptionsDoNotUse(sysroot); + } + public ImmutableList getLinkOptions() { - return cppConfiguration.getLinkOptions(sysroot); + return cppConfiguration.getLinkOptionsDoNotUse(/* sysroot= */ null); } + // Not all of CcToolchainProvider is exposed to Skylark, which makes implementing deep equality // impossible: if Java-only parts are considered, the behavior is surprising in Skylark, if they // are not, the behavior is surprising in Java. Thus, object identity it is. diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CompileCommandLine.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CompileCommandLine.java index 33d282e2b4..3215cbb36e 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CompileCommandLine.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CompileCommandLine.java @@ -39,7 +39,6 @@ public final class CompileCommandLine { private final CcToolchainFeatures.Variables variables; private final String actionName; private final CppConfiguration cppConfiguration; - private final CcToolchainProvider cppProvider; private final DotdFile dotdFile; private CompileCommandLine( @@ -50,8 +49,7 @@ public final class CompileCommandLine { CppConfiguration cppConfiguration, CcToolchainFeatures.Variables variables, String actionName, - DotdFile dotdFile, - CcToolchainProvider cppProvider) { + DotdFile dotdFile) { this.sourceFile = Preconditions.checkNotNull(sourceFile); this.outputFile = Preconditions.checkNotNull(outputFile); this.coptsFilter = coptsFilter; @@ -60,7 +58,6 @@ public final class CompileCommandLine { this.variables = variables; this.actionName = actionName; this.dotdFile = isGenerateDotdFile(sourceFile) ? Preconditions.checkNotNull(dotdFile) : null; - this.cppProvider = cppProvider; } /** Returns true if Dotd file should be generated. */ @@ -119,13 +116,6 @@ public final class CompileCommandLine { addFilteredOptions( options, featureConfiguration.getPerFeatureExpansions(actionName, updatedVariables)); - if (isObjcCompile(actionName)) { - PathFragment sysroot = cppProvider.getSysroot(); - if (sysroot != null) { - options.add(cppConfiguration.getSysrootCompilerOption(sysroot)); - } - } - if (!featureConfiguration.isEnabled("compile_action_flags_in_flag_set")) { if (FileType.contains(outputFile, CppFileTypes.ASSEMBLER, CppFileTypes.PIC_ASSEMBLER)) { options.add("-S"); @@ -142,11 +132,6 @@ public final class CompileCommandLine { return options; } - private boolean isObjcCompile(String actionName) { - return (actionName.equals(CppCompileAction.OBJC_COMPILE) - || actionName.equals(CppCompileAction.OBJCPP_COMPILE)); - } - // For each option in 'in', add it to 'out' unless it is matched by the 'coptsFilter' regexp. private void addFilteredOptions( List out, List>> expandedFeatures) { @@ -193,16 +178,9 @@ public final class CompileCommandLine { Predicate coptsFilter, String actionName, CppConfiguration cppConfiguration, - DotdFile dotdFile, - CcToolchainProvider cppProvider) { + DotdFile dotdFile) { return new Builder( - sourceFile, - outputFile, - coptsFilter, - actionName, - cppConfiguration, - dotdFile, - cppProvider); + sourceFile, outputFile, coptsFilter, actionName, cppConfiguration, dotdFile); } /** A builder for a {@link CompileCommandLine}. */ @@ -215,7 +193,6 @@ public final class CompileCommandLine { private final String actionName; private final CppConfiguration cppConfiguration; @Nullable private final DotdFile dotdFile; - private final CcToolchainProvider ccToolchainProvider; public CompileCommandLine build() { return new CompileCommandLine( @@ -226,8 +203,7 @@ public final class CompileCommandLine { Preconditions.checkNotNull(cppConfiguration), Preconditions.checkNotNull(variables), Preconditions.checkNotNull(actionName), - dotdFile, - Preconditions.checkNotNull(ccToolchainProvider)); + dotdFile); } private Builder( @@ -236,15 +212,13 @@ public final class CompileCommandLine { Predicate coptsFilter, String actionName, CppConfiguration cppConfiguration, - DotdFile dotdFile, - CcToolchainProvider ccToolchainProvider) { + DotdFile dotdFile) { this.sourceFile = sourceFile; this.outputFile = outputFile; this.coptsFilter = coptsFilter; this.actionName = actionName; this.cppConfiguration = cppConfiguration; this.dotdFile = dotdFile; - this.ccToolchainProvider = ccToolchainProvider; } /** Sets the feature configuration for this compile action. */ diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppActionConfigs.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppActionConfigs.java index 958b26f7f6..e965d4a82f 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppActionConfigs.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppActionConfigs.java @@ -68,6 +68,7 @@ public class CppActionConfigs { " }", " implies: 'legacy_compile_flags'", " implies: 'user_compile_flags'", + " implies: 'sysroot'", " implies: 'unfiltered_compile_flags'", "}", "action_config {", @@ -78,6 +79,7 @@ public class CppActionConfigs { " }", " implies: 'legacy_compile_flags'", " implies: 'user_compile_flags'", + " implies: 'sysroot'", " implies: 'unfiltered_compile_flags'", "}", "action_config {", @@ -88,6 +90,7 @@ public class CppActionConfigs { " }", " implies: 'legacy_compile_flags'", " implies: 'user_compile_flags'", + " implies: 'sysroot'", " implies: 'unfiltered_compile_flags'", "}", "action_config {", @@ -98,6 +101,7 @@ public class CppActionConfigs { " }", " implies: 'legacy_compile_flags'", " implies: 'user_compile_flags'", + " implies: 'sysroot'", " implies: 'unfiltered_compile_flags'", "}", "action_config {", @@ -108,6 +112,7 @@ public class CppActionConfigs { " }", " implies: 'legacy_compile_flags'", " implies: 'user_compile_flags'", + " implies: 'sysroot'", " implies: 'unfiltered_compile_flags'", "}", "action_config {", @@ -118,6 +123,7 @@ public class CppActionConfigs { " }", " implies: 'legacy_compile_flags'", " implies: 'user_compile_flags'", + " implies: 'sysroot'", " implies: 'unfiltered_compile_flags'", "}", "action_config {", @@ -128,6 +134,7 @@ public class CppActionConfigs { " }", " implies: 'legacy_compile_flags'", " implies: 'user_compile_flags'", + " implies: 'sysroot'", " implies: 'unfiltered_compile_flags'", "}", "action_config {", @@ -138,6 +145,7 @@ public class CppActionConfigs { " }", " implies: 'legacy_compile_flags'", " implies: 'user_compile_flags'", + " implies: 'sysroot'", " implies: 'unfiltered_compile_flags'", "}", ifTrue( @@ -376,6 +384,7 @@ public class CppActionConfigs { " implies: 'legacy_link_flags'", " implies: 'linker_param_file'", " implies: 'fission_support'", + " implies: 'sysroot'", "}", "action_config {", " config_name: 'c++-link-dynamic-library'", @@ -398,6 +407,7 @@ public class CppActionConfigs { " implies: 'legacy_link_flags'", " implies: 'linker_param_file'", " implies: 'fission_support'", + " implies: 'sysroot'", "}", "action_config {", " config_name: 'c++-link-static-library'", @@ -812,29 +822,6 @@ public class CppActionConfigs { " }", " }", "}"), - ifTrue( - !existingFeatureNames.contains("linker_param_file"), - "feature {", - " name: 'linker_param_file'", - " flag_set {", - " expand_if_all_available: 'linker_param_file'", - " action: 'c++-link-executable'", - " action: 'c++-link-dynamic-library'", - " flag_group {", - " flag: '-Wl,@%{linker_param_file}'", - " }", - " }", - " flag_set {", - " expand_if_all_available: 'linker_param_file'", - " action: 'c++-link-static-library'", - " action: 'c++-link-alwayslink-static-library'", - " action: 'c++-link-pic-static-library'", - " action: 'c++-link-alwayslink-pic-static-library'", - " flag_group {", - " flag: '@%{linker_param_file}'", - " }", - " }", - "}"), ifTrue( !existingFeatureNames.contains(CppRuleClasses.COVERAGE), "feature {", @@ -969,6 +956,28 @@ public class CppActionConfigs { " }", " }", "}"), + ifTrue( + !existingFeatureNames.contains("sysroot"), + "feature {", + " name: 'sysroot'", + " enabled: true", + " flag_set {", + " action: 'preprocess-assemble'", + " action: 'c-compile'", + " action: 'c++-compile'", + " action: 'c++-header-parsing'", + " action: 'c++-header-preprocessing'", + " action: 'c++-module-compile'", + " action: 'c++-link-executable'", + " action: 'c++-link-dynamic-library'", + " action: 'clif-match'", + " action: 'lto-backend'", + " flag_group {", + " expand_if_all_available: 'sysroot'", + " flag: '--sysroot=%{sysroot}'", + " }", + " }", + "}"), // unfiltered_compile_flags contain system include paths. These must be added // after the user provided options (present in legacy_compile_flags build // variable above), otherwise users adding include paths will not pick up their own @@ -995,6 +1004,29 @@ public class CppActionConfigs { " flag: '%{unfiltered_compile_flags}'", " }", " }", + "}"), + ifTrue( + !existingFeatureNames.contains("linker_param_file"), + "feature {", + " name: 'linker_param_file'", + " flag_set {", + " expand_if_all_available: 'linker_param_file'", + " action: 'c++-link-executable'", + " action: 'c++-link-dynamic-library'", + " flag_group {", + " flag: '-Wl,@%{linker_param_file}'", + " }", + " }", + " flag_set {", + " expand_if_all_available: 'linker_param_file'", + " action: 'c++-link-static-library'", + " action: 'c++-link-alwayslink-static-library'", + " action: 'c++-link-pic-static-library'", + " action: 'c++-link-alwayslink-pic-static-library'", + " flag_group {", + " flag: '@%{linker_param_file}'", + " }", + " }", "}"))); } 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 84a58026ad..0de697be74 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 @@ -345,8 +345,7 @@ public class CppCompileAction extends AbstractAction coptsFilter, actionName, cppConfiguration, - dotdFile, - cppProvider) + dotdFile) .setFeatureConfiguration(featureConfiguration) .setVariables(variables) .build(); 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 539883bb72..4133decc06 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 @@ -1139,31 +1139,40 @@ public class CppConfiguration extends BuildConfiguration.Fragment { /** * Returns the default list of options which cannot be filtered by BUILD rules. These should be * appended to the command line after filtering. + * + * @deprecated since it uses nonconfigured sysroot. Use + * {@link CcToolchainProvider#getUnfilteredCompilerOptionsWithSysroot(Iterable)} if you *really* + * need to. */ + // TODO(b/65401585): Migrate existing uses to cc_toolchain and cleanup here. + @Deprecated @SkylarkCallable( name = "unfiltered_compiler_options", doc = "Returns the default list of options which cannot be filtered by BUILD " + "rules. These should be appended to the command line after filtering." ) - public ImmutableList getUnfilteredCompilerOptions(Iterable features) { - return getUnfilteredCompilerOptions(features, nonConfiguredSysroot); + public ImmutableList getUnfilteredCompilerOptionsWithLegacySysroot( + Iterable features) { + return getUnfilteredCompilerOptionsDoNotUse(features, nonConfiguredSysroot); } - public ImmutableList getUnfilteredCompilerOptions( - Iterable features, PathFragment sysroot) { + /** + * @deprecated since it hardcodes --sysroot flag. Use + * {@link com.google.devtools.build.lib.rules.cpp.CcToolchainFeatures.FeatureConfiguration} + * instead. + */ + // TODO(b/65401585): Migrate existing uses to cc_toolchain and cleanup here. + @Deprecated + ImmutableList getUnfilteredCompilerOptionsDoNotUse( + Iterable features, @Nullable PathFragment sysroot) { if (sysroot == null) { return unfilteredCompilerFlags.evaluate(features); - } else { - return ImmutableList.builder() - .add(getSysrootCompilerOption(sysroot)) - .addAll(unfilteredCompilerFlags.evaluate(features)) - .build(); } - } - - public String getSysrootCompilerOption(PathFragment sysroot) { - return "--sysroot=" + sysroot; + return ImmutableList.builder() + .add("--sysroot=" + sysroot) + .addAll(unfilteredCompilerFlags.evaluate(features)) + .build(); } /** @@ -1171,8 +1180,11 @@ public class CppConfiguration extends BuildConfiguration.Fragment { * command-line options. * * @see Link + * @deprecated since it uses nonconfigured sysroot. Use + * {@link CcToolchainProvider#getLinkOptionsWithSysroot()} if you *really* need to. */ - // TODO(bazel-team): Clean up the linker options computation! + // TODO(b/65401585): Migrate existing uses to cc_toolchain and cleanup here. + @Deprecated @SkylarkCallable( name = "link_options", structField = true, @@ -1180,11 +1192,18 @@ public class CppConfiguration extends BuildConfiguration.Fragment { "Returns the set of command-line linker options, including any flags " + "inferred from the command-line options." ) - public ImmutableList getLinkOptions() { - return getLinkOptions(nonConfiguredSysroot); + public ImmutableList getLinkOptionsWithLegacySysroot() { + return getLinkOptionsDoNotUse(nonConfiguredSysroot); } - public ImmutableList getLinkOptions(PathFragment sysroot) { + /** + * @deprecated since it hardcodes --sysroot flag. Use + * {@link com.google.devtools.build.lib.rules.cpp.CcToolchainFeatures.FeatureConfiguration} + * instead. + */ + // TODO(b/65401585): Migrate existing uses to cc_toolchain and cleanup here. + @Deprecated + ImmutableList getLinkOptionsDoNotUse(@Nullable PathFragment sysroot) { if (sysroot == null) { return linkOptions; } else { diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionBuilder.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionBuilder.java index d4f3e4e515..2d47b74687 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionBuilder.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionBuilder.java @@ -1436,6 +1436,11 @@ public class CppLinkActionBuilder { buildVariables.addStringVariable(FORCE_PIC_VARIABLE, ""); } + if (toolchain.getSysroot() != null) { + buildVariables.addStringVariable( + CppModel.SYSROOT_VARIABLE_NAME, toolchain.getSysroot().getPathString()); + } + if (cppConfiguration.shouldStripBinaries()) { buildVariables.addStringVariable(STRIP_DEBUG_SYMBOLS_VARIABLE, ""); } 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 fa1253f618..72b65a983f 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 @@ -159,7 +159,10 @@ public final class CppModel { /** Build variable for flags coming from unfiltered_cxx_flag CROSSTOOL fields. */ public static final String UNFILTERED_COMPILE_FLAGS_VARIABLE_NAME = "unfiltered_compile_flags"; - + + /** Name of the build variable for the sysroot path variable name. */ + public static final String SYSROOT_VARIABLE_NAME = "sysroot"; + private final CppSemantics semantics; private final RuleContext ruleContext; private final BuildConfiguration configuration; @@ -641,6 +644,11 @@ public final class CppModel { LTO_INDEXING_BITCODE_FILE_VARIABLE_NAME, ltoIndexingFile.getExecPathString()); } + if (ccToolchain.getSysroot() != null) { + buildVariables.addStringVariable( + SYSROOT_VARIABLE_NAME, ccToolchain.getSysroot().getPathString()); + } + buildVariables.addAllStringVariables(ccToolchain.getBuildVariables()); buildVariables.addAllStringVariables(sourceSpecificBuildVariables); diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/LinkCommandLine.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/LinkCommandLine.java index 1943cddf88..4d4a6c9286 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/LinkCommandLine.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/LinkCommandLine.java @@ -547,16 +547,10 @@ public final class LinkCommandLine extends CommandLine { // Needed to find headers included from linkstamps. optionList.add("-I."); - // Add sysroot. - PathFragment sysroot = ccProvider.getSysroot(); - if (sysroot != null) { - optionList.add("--sysroot=" + sysroot.getPathString()); - } - // Add toolchain compiler options. optionList.addAll(cppConfiguration.getCompilerOptions(features)); optionList.addAll(cppConfiguration.getCOptions()); - optionList.addAll(ccProvider.getUnfilteredCompilerOptions(features)); + optionList.addAll(ccProvider.getUnfilteredCompilerOptionsWithSysroot(features)); if (CppFileTypes.CPP_SOURCE.matches(linkstamp.getKey().getExecPath())) { optionList.addAll(cppConfiguration.getCxxOptions(features)); } -- cgit v1.2.3