Skip to content

Commit

Permalink
Also move default_compile_flags to the top of the crosstool
Browse files Browse the repository at this point in the history 
Currently we move legacy_compile_flags to the top of the crosstool, and the same behavior is needed for migrated crosstools in case of default_compile_flags.

#6861
#5883

RELNOTES: None.
PiperOrigin-RevId: 229339668
  • Loading branch information
@hlopko
hlopko authored and Copybara-Service committed Jan 15, 2019
1 parent ee629ce commit ee8b357
Show file tree
Hide file tree
Showing 5 changed files with 125 additions and 22 deletions.
71 changes: 71 additions & 0 deletions site/docs/crosstool-reference.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1043,4 +1043,75 @@ conditions.
be enabled, `--force_pic` cannot be used.
</td>
</tr>
<tr>
<td>
<strong><code>static_linking_mode | dynamic_linking_mode</code></strong>
</td>
<td>Enabled by default based on linking mode.</td>
</tr>
<tr>
<td><strong><code>no_legacy_features</code></strong>
</td>
<td>
Prevents Bazel from adding legacy features to
the CROSSTOOL configuration when present. See the complete list of
features below.
</td>
</tr>
</table>

#### Legacy features patching logic

<p>
Bazel does following changes to the features and their order to stay backwards
compatible:

<ul>
<li>Moves <code>legacy_compile_flags</code> feature to the top of the toolchain</li>
<li>Moves <code>default_compile_flags</code> feature to the top of the toolchain</li>
<li>Adds <code>dependency_file</code> (if not present) feature to the top of the toolchain</li>
<li>Adds <code>pic</code> (if not present) feature to the top of the toolchain</li>
<li>Adds <code>per_object_debug_info</code> (if not present) feature to the top of the toolchain</li>
<li>Adds <code>preprocessor_defines</code> (if not present) feature to the top of the toolchain</li>
<li>Adds <code>includes</code> (if not present) feature to the top of the toolchain</li>
<li>Adds <code>include_paths</code> (if not present) feature to the top of the toolchain</li>
<li>Adds <code>fdo_instrument</code> (if not present) feature to the top of the toolchain</li>
<li>Adds <code>fdo_optimize</code> (if not present) feature to the top of the toolchain</li>
<li>Adds <code>fdo_prefetch_hints</code> (if not present) feature to the top of the toolchain</li>
<li>Adds <code>autofdo</code> (if not present) feature to the top of the toolchain</li>
<li>Adds <code>build_interface_libraries</code> (if not present) feature to the top of the toolchain</li>
<li>Adds <code>dynamic_library_linker_tool</code> (if not present) feature to the top of the toolchain</li>
<li>Adds <code>symbol_counts</code> (if not present) feature to the top of the toolchain</li>
<li>Adds <code>shared_flag</code> (if not present) feature to the top of the toolchain</li>
<li>Adds <code>linkstamps</code> (if not present) feature to the top of the toolchain</li>
<li>Adds <code>output_execpath_flags</code> (if not present) feature to the top of the toolchain</li>
<li>Adds <code>runtime_library_search_directories</code> (if not present) feature to the top of the toolchain</li>
<li>Adds <code>library_search_directories</code> (if not present) feature to the top of the toolchain</li>
<li>Adds <code>archiver_flags</code> (if not present) feature to the top of the toolchain</li>
<li>Adds <code>libraries_to_link</code> (if not present) feature to the top of the toolchain</li>
<li>Adds <code>force_pic_flags</code> (if not present) feature to the top of the toolchain</li>
<li>Adds <code>user_link_flags</code> (if not present) feature to the top of the toolchain</li>
<li>Adds <code>legacy_link_flags</code> (if not present) feature to the top of the toolchain</li>
<li>Adds <code>static_libgcc</code> (if not present) feature to the top of the toolchain</li>
<li>Adds <code>fission_support</code> (if not present) feature to the top of the toolchain</li>
<li>Adds <code>strip_debug_symbols</code> (if not present) feature to the top of the toolchain</li>
<li>Adds <code>coverage</code> (if not present) feature to the top of the toolchain</li>
<li>Adds <code>llvm_coverage_map_format</code> (if not present) feature to the top of the toolchain</li>
<li>Adds <code>gcc_coverage_map_format</code> (if not present) feature to the top of the toolchain</li>
<li>Adds <code>fully_static_link</code> (if not present) feature to the bottom of the toolchain</li>
<li>Adds <code>user_compile_flags</code> (if not present) feature to the bottom of the toolchain</li>
<li>Adds <code>sysroot</code> (if not present) feature to the bottom of the toolchain</li>
<li>Adds <code>unfiltered_compile_flags</code> (if not present) feature to the bottom of the toolchain</li>
<li>Adds <code>linker_param_file</code> (if not present) feature to the bottom of the toolchain</li>
<li>Adds <code>compiler_input_flags</code> (if not present) feature to the bottom of the toolchain</li>
<li>Adds <code>compiler_output_flags</code> (if not present) feature to the bottom of the toolchain</li>
</ul>
</p>

This is a long list of features. The plan is to get rid of them once
[Crosstool in Starlark](https://github.com/bazelbuild/bazel/issues/5380) is
done. For the curious reader see the implementation in
[CppActionConfigs](https://source.bazel.build/bazel/+/master:src/main/java/com/google/devtools/build/lib/rules/cpp/CppActionConfigs.java?q=cppactionconfigs&ss=bazel),
and for production toolchains consider adding `no_legacy_features` to make
the toolchain more standalone.

18 changes: 16 additions & 2 deletions src/main/java/com/google/devtools/build/lib/rules/cpp/CcModule.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -887,11 +887,24 @@ public CcToolchainConfigInfo ccToolchainConfigInfoFromSkylark(
// compile command line. 'legacy_compile_flags' feature contains all these flags, and so it
// needs to appear before other features from {@link CppActionConfigs}.
if (featureNames.contains(CppRuleClasses.LEGACY_COMPILE_FLAGS)) {
legacyFeaturesBuilder.add(
Feature legacyCompileFlags =
featureList.stream()
.filter(feature -> feature.getName().equals(CppRuleClasses.LEGACY_COMPILE_FLAGS))
.findFirst()
.get());
.get();
if (legacyCompileFlags != null) {
legacyFeaturesBuilder.add(legacyCompileFlags);
}
}
if (featureNames.contains(CppRuleClasses.DEFAULT_COMPILE_FLAGS)) {
Feature defaultCompileFlags =
featureList.stream()
.filter(feature -> feature.getName().equals(CppRuleClasses.DEFAULT_COMPILE_FLAGS))
.findFirst()
.get();
if (defaultCompileFlags != null) {
legacyFeaturesBuilder.add(defaultCompileFlags);
}
}

CppPlatform platform = targetLibc.equals("macos") ? CppPlatform.MAC : CppPlatform.LINUX;
Expand All @@ -907,6 +920,7 @@ public CcToolchainConfigInfo ccToolchainConfigInfoFromSkylark(
legacyFeaturesBuilder.addAll(
featureList.stream()
.filter(feature -> !feature.getName().equals(CppRuleClasses.LEGACY_COMPILE_FLAGS))
.filter(feature -> !feature.getName().equals(CppRuleClasses.DEFAULT_COMPILE_FLAGS))
.collect(ImmutableList.toImmutableList()));
for (CToolchain.Feature feature :
CppActionConfigs.getFeaturesToAppearLastInFeaturesList(featureNames)) {
Expand Down
7 changes: 7 additions & 0 deletions src/main/java/com/google/devtools/build/lib/rules/cpp/CppRuleClasses.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -223,6 +223,13 @@ public static Label ccToolchainTypeAttribute(RuleDefinitionEnvironment env) {
*/
public static final String LEGACY_COMPILE_FLAGS = "legacy_compile_flags";

/**
* A string constant for the default_compile_flags feature. If this feature is present in the
* toolchain, and the toolchain doesn't specify no_legacy_features, bazel will move
* default_compile_flags before other features from {@link CppActionConfigs}.
*/
public static final String DEFAULT_COMPILE_FLAGS = "default_compile_flags";

/**
* A string constant for the feature that makes us build per-object debug info files.
*/
Expand Down
43 changes: 26 additions & 17 deletions src/main/java/com/google/devtools/build/lib/rules/cpp/CppToolchainInfo.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -405,22 +405,32 @@ public static CToolchain addLegacyFeatures(
}
}

// TODO(b/30109612): Remove fragile legacyCompileFlags shuffle once there are no legacy
// crosstools.
// Existing projects depend on flags from legacy toolchain fields appearing first on the
// compile command line. 'legacy_compile_flags' feature contains all these flags, and so it
// needs to appear before other features from {@link CppActionConfigs}.
CToolchain.Feature legacyCompileFlagsFeature =
toolchain
.getFeatureList()
.stream()
// TODO(b/30109612): Remove fragile legacyCompileFlags shuffle once there are no legacy
// crosstools.
// Existing projects depend on flags from legacy toolchain fields appearing first on the
// compile command line. 'legacy_compile_flags' feature contains all these flags, and so it
// needs to appear before other features from {@link CppActionConfigs}.
if (featureNames.contains(CppRuleClasses.LEGACY_COMPILE_FLAGS)) {
CToolchain.Feature legacyCompileFlags =
toolchain.getFeatureList().stream()
.filter(feature -> feature.getName().equals(CppRuleClasses.LEGACY_COMPILE_FLAGS))
.findFirst()
.orElse(null);
if (legacyCompileFlagsFeature != null) {
toolchainBuilder.addFeature(legacyCompileFlagsFeature);
toolchain = removeLegacyCompileFlagsFeatureFromToolchain(toolchain);
.get();
if (legacyCompileFlags != null) {
toolchainBuilder.addFeature(legacyCompileFlags);
}
}
if (featureNames.contains(CppRuleClasses.DEFAULT_COMPILE_FLAGS)) {
CToolchain.Feature defaultCompileFlags =
toolchain.getFeatureList().stream()
.filter(feature -> feature.getName().equals(CppRuleClasses.DEFAULT_COMPILE_FLAGS))
.findFirst()
.get();
if (defaultCompileFlags != null) {
toolchainBuilder.addFeature(defaultCompileFlags);
}
}
toolchain = removeSpecialFeatureFromToolchain(toolchain);

CppPlatform platform =
toolchain.getTargetLibc().equals("macosx") ? CppPlatform.MAC : CppPlatform.LINUX;
Expand Down Expand Up @@ -452,16 +462,15 @@ public static CToolchain addLegacyFeatures(
return toolchainBuilder.build();
}

private static CToolchain removeLegacyCompileFlagsFeatureFromToolchain(CToolchain toolchain) {
private static CToolchain removeSpecialFeatureFromToolchain(CToolchain toolchain) {
FieldDescriptor featuresFieldDescriptor = CToolchain.getDescriptor().findFieldByName("feature");
return toolchain
.toBuilder()
.setField(
featuresFieldDescriptor,
toolchain
.getFeatureList()
.stream()
toolchain.getFeatureList().stream()
.filter(feature -> !feature.getName().equals(CppRuleClasses.LEGACY_COMPILE_FLAGS))
.filter(feature -> !feature.getName().equals(CppRuleClasses.DEFAULT_COMPILE_FLAGS))
.collect(ImmutableList.toImmutableList()))
.build();
}
Expand Down
8 changes: 5 additions & 3 deletions src/test/java/com/google/devtools/build/lib/rules/cpp/SkylarkCcCommonTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4267,6 +4267,7 @@ public void testCcToolchainInfoFromSkylarkWithLegacyFeatures() throws Exception
" feature(name = 'custom_feature'),",
" feature(name = 'legacy_compile_flags'),",
" feature(name = 'fdo_optimize'),",
" feature(name = 'default_compile_flags'),",
" ],",
" action_configs = [action_config(action_name = 'custom-action')],",
" artifact_name_patterns = [artifact_name_pattern(",
Expand Down Expand Up @@ -4307,10 +4308,11 @@ public void testCcToolchainInfoFromSkylarkWithLegacyFeatures() throws Exception
.collect(ImmutableSet.toImmutableSet());
// fdo_optimize should not be re-added to the list of features by legacy behavior
assertThat(featureNames).containsNoDuplicates();
// legacy_compile_flags should appear first in the list of features
assertThat(featureNames.get(0)).isEqualTo("legacy_compile_flags");
// legacy_compile_flags should appear first in the list of features, followed by
// default_compile_flags.
assertThat(featureNames)
.containsAllOf("legacy_compile_flags", "custom_feature", "fdo_optimize")
.containsAllOf(
"legacy_compile_flags", "default_compile_flags", "custom_feature", "fdo_optimize")
.inOrder();
// assemble is one of the action_configs added as a legacy behavior, therefore it needs to be
// prepended to the action configs defined by the user.
Expand Down

0 comments on commit ee8b357

Please sign in to comment.