Bazel: incompatible_disallow_data_transition: remove outdated "cfg = "data'" syntax

[gregestren]

Description of the problem / feature request:

cfg = "data" is outdated syntax that was turned into a no-op in https://github.com/bazelbuild/bazel/commit/10a6130dbafdf4e5f61198bec08b88fe6d6381fe.

The functionality this was originally intended for will be provided by Starlark build configuration.

Feature requests: what underlying problem are you trying to solve with this feature?

Remove outdated code & clean up technical debt.

Bugs: what's the simplest, easiest way to reproduce this bug? Please provide a minimal example if possible.

Support can already be toggled with --incompatible_disallow_data_transition:

$ cat testapp/rules.bzl
def _foo_rule_impl(ctx):
    return []

foo_rule = rule(
    implementation = _foo_rule_impl,
    attrs = {
        "data": attr.label_list(cfg = "data"),
    },
)

$ cat testapp/BUILD
load("//testapp:rules.bzl", "foo_rule")

foo_rule(
    name = "g",
)

$ bazel build //testapp:g --incompatible_disallow_data_transition=false  [default]
INFO: Build completed successfully, 1 total action

$ bazel build //testapp:g --incompatible_disallow_data_transition
ERROR: /workspace/testapp/rules.bzl:7:17: Traceback (most recent call last):
    File "/workspace/testapp/rules.bzl", line 4
        rule(implementation = _foo_rule_impl, a...")})
    File "/workspace/testapp/rules.bzl", line 7, in rule
        attr.label_list(cfg = "data")
Using cfg = "data" on an attribute is a noop and no longer supported. Please remove it.
You can use --incompatible_disallow_data_transition=false to temporarily disable this
check.
ERROR: Skipping '//testapp:g': error loading package 'testapp': Extension file
'testapp/rules.bzl' has errors
FAILED: Build did NOT complete successfully (1 packages loaded)

[gregestren]

Any recommendations on a deprecation plan?

[c-parsons]

vladmos@, might be a good candidate for our upcoming formatter/migrator tool. This one's a pretty easy one -- we just need to remove all instances of cfg = "data". They're a no-op.

[vladmos]

c-parsons@ I've already implemented this particular fix, it searches for nodes that match the pattern attr.xxxx(..., cfg = "data", ...) and report/remove the cfg = "data" part.

[gregestren]

How does this catch all uses across all repositories?

i.e. how do we know when its safe to enforce the deprecation?

[laurentlb]

We will never know: not all the users of Bazel publish the BUILD/bzl files.

To provide smooth upgrade for users:

• Give a notice (with the incompatible flag + note in the release announcement)
• Make sure that all projects tested on the CI are not broken by the change
• Provide a tool

Once the tool has a few more fixes (for other kinds of cleanup), we can advertise it widely.

[c-parsons]

vladmos@: awesome! Is there a good documentation source that we can point users to for using the tool to fix this?
I can draft a notice, but I want to make sure to link to the tool :)

[dslomov]

What is missing for migration: migration docs, length of migration window. After these are done, please add "migration-ready" label.

[vladmos]

Can be automatically fixed with buildifier --lint=fix --warnings=attr-cfg

[meteorcloudy]

Flipping this flag will causing many projects to fail in downstream, please help the downstream projects to migrate (filing issues, pinging the project owner, etc)
https://buildkite.com/bazel/bazel-at-release-plus-incompatible-flags/builds/13#37dd9512-9d32-4430-befb-ce3d371014e7

This is flag was planned to release in 0.21.0, but it's not flipped when 0.21.0 was cut, so looks like we should extend the migration window.

[meteorcloudy]

This flag will break many projects in downstream, you can see the details at
https://buildkite.com/bazel/bazel-at-release-plus-incompatible-flags/builds/48

Please help them migrate (by filing issues and sending PRs and pinging the owners), thanks!

[gregestren]

I'm happy to help migrate. Is anyone else signed up for helping? Any tracking issue to follow the TODOs?

[vladmos]

I tried to migrate earlier today, many of the projects from the link above have already been fixed, I've sent https://github.com/bazelbuild/intellij/pull/485 for review. Would it be possible to rerun the global presubmit with --incompatible_disallow_data_transition again?

[gregestren]

So sounds like you're on top of it. Let me know if you want any help.

[meteorcloudy]

@vladmos Yes, you can rerun the pipeline by clicking "New Build" at https://buildkite.com/bazel/bazel-at-release-plus-incompatible-flags

[laurentlb]

I should have updated this thread before. I've been sending PRs over the last days/weeks. My Tensorflow PR was merged this night. I think IntelliJ/CLion is the last one.
Thanks Vlad!

[meteorcloudy]

Thanks everyone!
@laurentlb TensorFlow's own build is still red (due to Windows build). Because we test downstream at last green commit, so your fix is not yet in downstream. I'm asking TF to fix their Windows build, then it will also be green in downstream with incompatible changes.

[philwo]

I verified that this flag was flipped and thus will be included in the Bazel 0.23.0 release.