incompatible_disable_legacy_crosstool_fields: Disable legacy crosstool fields

[hlopko]

Flag: --incompatible_disable_legacy_crosstool_fields
Available since: 0.22 (January 2019 release)
Will be flipped in: 0.25 (April 2019 release)
Tracking issue: #5883
Rollout doc: https://docs.google.com/document/d/1uv4c1zag6KvdI31qdx8C6jiTognXPQrxgsUpVefm9fM/edit#

Motivation

CROSSTOOL grew much bigger than it was expected in 2011 when it was first introduced. There are many fields that are no longer used (e.g. mao_plugin_header_directory), or there are fields that are superseded by features (e.g. compiler_flag). There are fields that specify toolchain capabilities (e.g. supports_embedded_runtimes) but cannot participate in the feature configuration (and there are user requests for these). All in all, CROSSTOOL needs a revamp, and this change achieves that.

Caveats

This is a big incompatible change, and #5380 will be comparably big one. We advice users to use the migration tool provided for --incompatible_disable_legacy_crosstool_fields as the final step of their CROSSTOOL generating pipeline, and wait with hand-tweaking for the migration tool that will be provided for #5380. --incompatible_disable_legacy_crosstool_fields will not be flipped before #5380 is fixed.

Migration

Please use the legacy_fields_migrator tool provided at rules_cc repository. To verify the correctness of your build, use the command line diffing tool provided at tools/aquery_differ (bazel aquery, that the differ uses, is being improved daily as of January 2019, e.g. 1, so if you don't use bazel > 0.23, we advice to build your own bazel from HEAD for testing the migrated crosstool). As mentioned above, we advice not to hand-tune the CROSSTOOL without also migrating for #5380 (automated migrator to starlark will be provided).

Detailed change description

These features were previously enabled by Bazel, but they will no longer be, so they need to be enabled in the crosstool:

• dependency_file
• random_seed
• module_maps
• module_map_home_cwd
• header_module_compile
• include_paths,
• pic
• preprocessor_defines

This list contains all the legacy fields and how they will be migrated:

• static_runtimes_filegroup will not be used once #6942 is flipped. Use cc_toolchain.static_runtime_lib instead.
• dynamic_runtimes_filegroup will not be used once #6942 is flipped. Use cc_toolchain.dynamic_runtime_lib instead.
• compiler_flag is replaced by features. Flags from compiler_flag should appear before any other legacy field, and should be passed to all compilation actions.
• optional_compiler_flag is not used.
• cxx_flag is replaced by features. Flags from cxx_flag should appear after compiler_flags, and should be passed to all compilation actions except c-compile.
• optional_cxx_flag is not used.
• unfiltered_cxx_flag is replaced by features. Flags from feature named unfiltered_compile_flags are not subject to nocopts filtering.
• optional_unfiltered_cxx_flag is not used.
• linker_flag is replaced by features. Flags from linker_flag should appear before any other legacy field, and should be passed to all linking actions (except the misnamed archiving action c++-link-static-library)
• optional_linker_flag is not used.
• dynamic_library_linker_flag is replaced by features. Flags from dynamic_library_linker_flag should appear after linker_flag fields from linking_mode_flags, and should be only present when linking dynamic libraries.
• optional_dynamic_library_linker_flag is not used.
• test_only_linker_flag is replaced by features. test_only_linker_flag should appear after dynamic_library_linker_flag, and only for tests.
• objcopy_embed_flag is replaced by features.
• ld_embed_flag is replaced by features.
• ar_flag is not used.
• ar_thin_archives_flag is not used.
• gcc_plugin_compiler_flag is not used.
• compilation_mode_flags are replaced by features. Features named opt, dbg, and fastbuild are automatically enabled for given compilation mode. Flags from inner compiler_flag should appear after top-level cxx_flags. Flags from inner cxx_flag should appear after inner compiler_flag. Flags from linker_flag should appear after top-level linker_flag fields, before flags from linking_mode_flags.
• lipo_mode_flags is not used.
• linking_mode_flags are replaced by features. Mode COVERAGE is not used. Mode FULLY_STATIC has been replaced by fully_static_link feature and therefore unused. Mode MOSTLY_STATIC is replaced by static_linking_mode feature and mode DYNAMIC is replaced by dynamic_linking_mode feature. Both are enabled by rules explicitly.
• gcc_plugin_header_directory is not used.
• mao_plugin_header_directory is not used.
• default_python_top is not used.
• default_python_version is not used.
• python_preload_swigdeps is not used.
• supports_gold_linker is only used for --symbol_counts and will be removed.
• supports_thin_archives is not used or user.
• supports_start_end_lib is replaced by feature named supports_start_end_lib that should be enabled by the toolchain or user.
• supports_interface_shared_objects is replaced by feature named supports_interface_shared_libraries that should be enabled by the toolchain or user.
• supports_embedded_runtimes is replaced by feature named static_link_cpp_runtimes that should be enabled by the toolchain or user.
• supports_incremental_linker is not used.
• supports_normalizing_ar is not used.
• supports_fission is replaced by feature named per_object_debug_info that should be enabled by the toolchain or user.
• supports_dsym is not used.
• needsPic is replaced by feature named supports_pic that should be enabled by the toolchain or user. Before Bazel always requrested pic feature, and verified that pic feature is enabled when needsPic was true or when --force_pic option was passed. With this change, Bazel will only request supports_pic when --force_pic is passed, and it verify that supports_pic is enabled only when --force_pic is passed. When --force_pic is not passed, Bazel will not request supports_pic explicitly, and it will assume that when supports_pic is enabled by the toolchain or user, it means PIC is needed for shared libraries. pic and force_pic build variables are still passed in their corresponding configurations.

A really good place to see exactly what is migrated and how is to see the tests for legacy_fields_migrator.

Changelog

• January 3rd: Implemented cmd_line_differ
• January 7th: Bazel 0.22, which contains this flag, was cut
• January 7th: Fixed broken interface library input detection (a4529db), this is unlikely to affect Bazel users.
• January 7th: Discovered the issue where $EXEC_ORIGIN is not used when static_link_cpp_runtimes is disabled, this is unlikely to affect Bazel users. If it does affect you, the fix was to add runtime_library_search_directories to the crosstool and not relying on Bazel to patch it in.
• January 11th: Open-sourced legacy_fields_migrator
• January 15th: Bazel fix to move default_compile_flags feature to the top of the crosstool, to stay backwards compatible. This fix does affect every user that doesn't specify no_legacy_features feature. Migrator fixed accordingly.
• January 17th: Migrator fix for nodeps_dynamic_library.
• February 5th: Migrator fix to replace repeated expand_if_(all|none)_available with nesting (preparation for Crosstool in Starlark migration).
• February 8th: Replaced cmd_line_differ with aquery_differ, which is a more general aquery output diffing tool. (3575797)
• February 13th: Updated the migrator to auto-enable some features that were previously enabled by Bazel

[hlopko]

CC @scentini

[nlopezgi]

Hi Marcel,
Were running some tests with bazel 0.22.0 and noticed that just running:
bazel build --all_incompatible_changes @local_config_cc//...
results in errors related to these changes.
i.e., it seems the default toolchain produced by local_config_cc is not compatible with using --all_incompatible_changes.
Is this expected? When can we have a local_config_cc that is compatible with the new flags (is that what #5380 is about?). Thanks!

[hlopko]

Hi Nick,

We only migrated bazel's crosstools in 0.23, since the flag will be flipped in 0.24. Also, in general, don't use --all_incompatible_changes (we should totally remove that), only look into which flags are in the migration window and/or which flags will be flipped in the next release.

This flag doesn't have the breaking change label yet, we don't know if we will be able to submit the flag for #5380 in time for 0.23. If not, we'll postpone flipping this one till 0.25.

Thanks for testing this!

[hlopko]

@dslomov , can you pls elaborate on why we removed the migration-ready label? People can already migrate their custom crosstools with bazel 0.22, and I don't expect many will be affected by bugs discovered so far. And I was hoping to get user feedback from 0.22.

[dslomov]

@hlopko no need to worry - your migrations are ongoing. Description of "migration-ready" label: Incompatible changes that are ready for migration, but migration window hasn't yet started

For this issue, migration has already started - they are marked with "migration-0.22" and "migration-0.23" labels, so I removed the "migration-ready" label. See Communicating Incompatible Changes for more details. (I know we haven't followed this quite so well up so far, but I am working on changing that, e.g. bazelbuild/continuous-integration#460)

[dslomov]

Also added "breaking-change-0.24" label to communicate your intended breakage timeframe. Please update once you have more information.

[nlopezgi]

thanks for the clarifications Marcel!

[tmc]

Hi Nick,

We only migrated bazel's crosstools in 0.23, since the flag will be flipped in 0.24. Also, in general, don't use --all_incompatible_changes (we should totally remove that), only look into which flags are in the migration window and/or which flags will be flipped in the next release.

This flag doesn't have the breaking change label yet, we don't know if we will be able to submit the flag for #5380 in time for 0.23. If not, we'll postpone flipping this one till 0.25.

Thanks for testing this!

As I recall there is published material that encourages running with --all_incompatible_changes (bazelcon talk from Laurent IIRC). I've encoded this into a project template and am running into the same issue @nlopezgi refers to above (incompatible local_config_cc).

[hlopko]

There was (and still is) a confusion re --all_incompatible_changes. It's an old option that has been superseded by the new process documented at https://docs.bazel.build/versions/master/backward-compatibility.html. I can see how it would be useful to have a option like --all_incompatible_changes_that_will_be_flipped_in_next_release, but that's not implemented yet.

One very important aspect of the new process is that an incompatible change can be created, github issue opened, and then after the community pushback abandoned. Such change would never get migration-ready, migration-x.y, or breaking-change-x.y+1 labels, so it will never break anybody. But --all_incompatible_changes will flip it and incorrectly report failures.

I'll see if we can remove --all_incompatible_changes to prevent further confusion. CC @laurentlb @dslomov.

[tmc]

I see. Thanks for the clarification.

[nlopezgi]

Thanks for the additional clarifications Marcel,
It does seem that --all_incompatible_changes is not what we need (and does not seem to make too much sense to have that option). However, I do think there is value to having some flag of the sort --all_incompatible_changes_that_are_migration_ready so that rule owners and project owners that want to make sure that all upcoming changes will be compatible with their code can test before an upcoming release.

[hlopko]

Yup I agree, I'm looking into this these days, and once I find a free afternoon I might add this behavior to bazelisk (to check github for incompatible flag issues, get the list, and pass them to bazel). I'll share details to bazel-discuss when I get there.

[dslomov]

Adding such options to bazelisk lgtm.

[Profpatsch]

It's an old option that has been superseded by the new process documented at https://docs.bazel.build/versions/master/backward-compatibility.html

@hlopko I cannot read from that documentation that incompatible flags could introduce unnecessary work. Can you add a recommended workflow for rules authors to that?
Because in theory it would be nice for rules authors to support coming breaking changes before they are flipped, so in my mind the --all_incompatible_changes (with selected changes disabled) had a lot of merit and provided a good way for us to be sure about future breaking changes. Removing it (or changing the semantics to “might be rolled back anyway trololo”) takes that away.

I understand that --strict_action_env was a mistake and had to be rolled back, but aren’t we throwing out the baby with the bathwater here?

[hlopko]

Hey @Profpatsch, I think we are all in agreement here, I'm just saying that --all_incompatible_changes is not a perfect solution. The perfect solution is to look into the github issues labelled with the migration-x.y or breaking-change-x.y, and only flip those incompatible flags. This can currently be done only manually (or by using bazel CI, where we have a pipeline for this (currently quite red :) : https://buildkite.com/bazel/bazel-at-release-plus-incompatible-flags/builds/96

While I don't think bazel should go and check github issues, bazelisk sounds like a good place for this logic to be in.

[mboes]

Bazelisk duplicates the core functionality of Nix. That's great for users who don't already use Nix or don't want to use Nix, but for those who do, there would be little reason to use Bazelisk. So I think a flag that flips exactly those changes that will be BC in the next release, as you suggest (and @nlopezgi seems to want too), would be very useful to have in Bazel itself. That way both Bazelisk and Nix users can benefit from it.

[dslomov]

Bazelisk duplicates the core functionality of Nix. That's great for users who don't already use Nix or don't want to use Nix, but for those who do, there would be little reason to use Bazelisk. So I think a flag that flips exactly those changes that will be BC in the next release, as you suggest (and @nlopezgi seems to want too), would be very useful to have in Bazel itself. That way both Bazelisk and Nix users can benefit from it.

The source-of-truth for that information is GitHub issues. Our intentions about when we are going to flip flags can change based on user feedback (and are reflected in labels on GitHub issues as documented in https://docs.bazel.build/versions/master/backward-compatibility.html).

We will certainly not bake such information into the Bazel binary itself - are you suggesting Bazel itself should call out to GitHub and check the flags? That would be non-hermetic.

I would like to re-iterate again: do not run your production builds with any --incompatible_* or --experimental_* flags. Test with the set of flags we plan flip in the next release, provide feedback etc - but you should be able at any time to migrate your code in preparation to the next release so that it works without any flags with the current and the next release.

As such, your users (who presumably get Bazel and your rules through Nix) will never need to run any builds with --incompatible_* or --experimental_* flags.

[Profpatsch]

From an IRC discussion:

Profpatsch: My reasoning is: they are moving really fast and breaking stuff, which is the right way to go.
But we can’t move as fast with some clients, e.g. because we need to wait for other parts of the ecosystem to catch up.
e.g. nixpkgs is always lagging behind a bit.
So having a guarantee that we are already compatible with the next version is a big advantage. We can fix issues at our own pace, instead of being driven by “oh, bazel xy came out yesterday, and client z really wants that now, so we have to fix these ten incompatible things today”

[dslomov]

@Profpatsch I am not quite sure which way

From an IRC discussion:

Profpatsch: My reasoning is: they are moving really fast and breaking stuff, which is the right way to go.
But we can’t move as fast with some clients, e.g. because we need to wait for other parts of the ecosystem to catch up.
e.g. nixpkgs is always lagging behind a bit.
So having a guarantee that we are already compatible with the next version is a big advantage. We can fix issues at our own pace, instead of being driven by “oh, bazel xy came out yesterday, and client z really wants that now, so we have to fix these ten incompatible things today”

I am not quite sure which way this argues....

[Profpatsch]

am not quite sure which way this argues

Having --all_incompatible_changes in the ruleset’s .bazelrc and fixing all errors that happen when switching the ruleset to a new bazel version, thus ensuring forward compatibility with the next bazel version. If errors are too hard to fix, selectively disable with their respective flags and report bugs.

[laurentlb]

There is the label migration-0.22, this means I am supposed to test this flag when I use Bazel 0.22?
It fails even on trivial projects:

$ touch WORKSPACE
$ echo 'cc_library(name = "x")' > BUILD
$ bazel build //... --incompatible_disable_legacy_crosstool_fields
ERROR: /mypath/.cache/bazel/_bazel_laurentlb/d1309ac1e2719cdf777c0d6e936fc92b/external/local_config_cc/BUILD:47:1: in cc_toolchain_suite rule @local_config_cc//:toolchain: compilation_mode_flags is disabled by --incompatible_disable_legacy_crosstool_fields, please migrate your CROSSTOOL (see https://github.com/bazelbuild/bazel/issues/6861 for migration instructions).

Regarding --all_incompatible_changes, the flag is not perfect, but I like to use it as a rough approximation of what I shall fix. I'll be happy to have a more precise replacement.

[joeleba]

3575797
tools/cmd_line_differ has been replaced with tool/aquery_differ, which is an extension of the old script that allows the diffing of other attributes of an action, in addition to its command line.

Changes:

• A new --attrs flag that specifies which attributes of an action you want to compare. --attrs has the default value of ["cmdline"]. --attrs currently takes the following values: (cmdline|inputs). We will introduce more attributes in the future.
• Formatting of the output

Example:

bazel run //tools/aquery_differ:aquery_differ -- \
--before=/path/to/output_one.textproto \
--after=/path/to/output_two.textproto
--input_type=textproto
--attrs=cmdline
--attrs=inputs

The above command would compare the diff in cmdline and inputs of the actions in the 2 input files.

[philwo]

Same problem here - this flag causes bazel build to fail even for projects that don't do anything special. It seems like the auto-generated @local_config_cc//:toolchain is not compatible with the flag, which is nothing the user can fix, right?

(Tested this on a Mac with macOS 10.14.3 and Xcode 10.1)

[mboes]

As such, your users (who presumably get Bazel and your rules through Nix) ...

@dslomov so does our CI.

will never need to run any builds with --incompatible_* or --experimental_* flags.

We have the following requirement:

• whenever our CI is using Bazel N (provisioned via Nix), we want our CI to tell us whether our rule set in compatible with N+1. That way we don't need to scramble at the last minute when Bazel N+1 is released to make our rule set compatible. We furthermore want to ensure that if we are compatible with N+1, that it stays that way until N+1 is released.

So what is the recommended way to achieve that? We were using --all_incompatible_changes previously to ensure this. What do you recommend that we use now?

[hlopko]

Let me remove the migration label then, my original thinking was it's better to have longer migration window, especially with a flag like this one, than smooth and false positives-free automation. I apologize for that.

[philwo]

So what is the recommended way to achieve that? We were using --all_incompatible_changes previously to ensure this. What do you recommend that we use now?

@mboes Because the source of truth regarding which flags will be flipped with the next release are GitHub labels in our issue tracker, the recommended way is to query GitHub and parse the flag names via a regex from the titles of the returned issues.

Here's some example code: https://github.com/philwo/bazelisk/blob/master/bazelisk.go#L341

Querying the GitHub API comes with fun problems like occasionally failing requests and rate limiting - if you have a suggestion how we could make this easier for users, please say so. Maybe publishing the list of flags in a file on a server somewhere?

[mboes]

This sounds a tad overengineered to me. Am I getting this right that after Bazel N is released, the worst you will do is not flip the switch on incompatible changes you announced you would make in the N+1 release? Presumably you would never flip the switch on a change you did not announce, or that would be violation of the policy documented here.

If this is correct, then the set of incompatible changes announced in the N changelog for the N+1 release is at worse a slight over approximation of what breaking changes will really occur, but never and an under approximation. We can live with one or two over approximations. What is much harder is our tooling only knowing at the very last minute that the release that was just cut breaks our rule set without writing tools to query the GitHub API or tools that parse changelogs.

I get that GitHub is the real source of truth. But what's wrong with the simple --all_incompatible_changes_that_are_migration_ready flag that @hlopko suggested, i.e. a flag that just reflects whatever was announced in the Bazel N release notes for the N+1 release? We can live with small over approximations of the real truth.

[hlopko]

I actually don't suggest not using github as a source of truth. When we remove migration-x.y label from the incompatible flag issue it means that the migration is not recommended. This is an useful signal to our users and not listening to it can result it wasted time. Maybe there is a bug in bazel that prevents you to migrate, maybe we received a pushback from the community and the migration is abandoned. In the meantime @laurentlb implemented philwo/bazelisk@e14ab80, and I think that's a really good solution to this problem.

[hlopko]

We discovered a bug in the migrated cc_configure, therefore we still cannot recommend users to migrate. We are postponing the migration window to 0.24, and the breaking release will be 0.25. We apologize for inconvenience.

[hlopko]

This broke bazel's ndk crosstools, reopening.