incompatible_disallow_struct_provider_syntax: disallow rule implementation functions returning struct

[c-parsons]

Flag: --incompatible_disallow_struct_provider_syntax
Available since: 0.23 (February 2019 release)

Motivation
The motivation for this change is described in #6241

Migration
Rule implementation functions should return a collection of provider instances instead of a struct

# Before
def _rule_impl(ctx):
    return struct(
        foo_info = struct(foo = ‘foo’),
        bar_info = struct(bar = ‘bar’),
    )

my_rule = rule(implementation = _rule_impl)

# After
FooInfo = provider()
BarInfo = provider()
def _rule_impl(ctx):
    return [FooInfo(foo = ‘foo’), BarInfo(bar = ‘bar’)]

my_rule = rule(implementation = _rule_impl)

One corner case to note is specifying files for coverage. This used
to be done with the hardcoded struct field instrumented_files, but should
now be done by using coverage_common.instrumented_files_info(). For example:

# Before
def rule_impl(ctx):
    return struct(
        instrumented_files = struct(
            source_attributes = ["srcs"],
            dependency_attributes = ["deps", "image"],
            extensions = ["js"],
        ),
    )

my_rule = rule(implementation = _rule_impl)

# After
def rule_impl(ctx):
    my_coverage_info = coverage_common.instrumented_files_info(
        ctx,
        source_attributes = ["srcs"],
        dependency_attributes = ["deps", "image"],
        extensions = ["js"])
    return [my_coverage_info]

my_rule = rule(implementation = _rule_impl)

[davido]

Gerrit Code Review seems to be affected:

INFO: Build options --incompatible_allow_python_version_transitions, --incompatible_disable_crosstool_file, --incompatible_disable_expand_if_all_available_in_flag_set, and 2 more have changed, discarding analysis cache.
ERROR: /home/davido/projects/gerrit2/lib/js/BUILD:24:1: in _bower_component rule //lib/js:webcomponentsjs:

/home/davido/projects/gerrit2/tools/bzl/js.bzl:149:12: Returning a struct from a rule implementation function is deprecated and will be removed soon. It may be temporarily re-enabled by setting --incompatible_disallow_struct_provider_syntax=false . See https://github.com/bazelbuild/bazel/issues/7347 for details.
ERROR: Analysis of target '//:release' failed; build aborted: Analysis of target '//lib/js:webcomponentsjs' failed; build aborted
INFO: Elapsed time: 1.166s
INFO: 0 processes.
FAILED: Build did NOT complete successfully (44 packages loaded, 876 targets configured)

[davido]

Now, it seems that Bazel at HEAD (e81d9c9) itself affected:

$ b10 build --all_incompatible_changes :gerrit
INFO: Invocation ID: c81793a2-5e7f-4f02-9b26-a84442e09a4c
DEBUG: /home/davido/.cache/bazel/_bazel_davido/5c01f4f713b675540b8b424c5c647f63/external/bazel_skylib/lib/versions.bzl:98:7: 
Current Bazel is not a release version, cannot check for compatibility.
DEBUG: /home/davido/.cache/bazel/_bazel_davido/5c01f4f713b675540b8b424c5c647f63/external/bazel_skylib/lib/versions.bzl:99:7: Make sure that you are running at least Bazel 0.22.0.
INFO: Build options --incompatible_allow_python_version_transitions, --incompatible_auto_configure_host_platform, --incompatible_cc_coverage, and 22 more have changed, discarding analysis cache.
ERROR: /home/davido/.cache/bazel/_bazel_davido/5c01f4f713b675540b8b424c5c647f63/external/bazel_tools/tools/jdk/BUILD:59:1: in java_toolchain_alias rule @bazel_tools//tools/jdk:current_java_toolchain: 


/home/davido/.cache/bazel/_bazel_davido/5c01f4f713b675540b8b424c5c647f63/external/bazel_tools/tools/jdk/java_toolchain_alias.bzl:85:12: Returning a struct from a rule implementation function is deprecated and will be removed soon. It may be temporarily re-enabled by setting --incompatible_disallow_struct_provider_syntax=false . See https://github.com/bazelbuild/bazel/issues/7347 for details.
ERROR: Analysis of target '//:gerrit' failed; build aborted: Analysis of target '@bazel_tools//tools/jdk:current_java_toolchain' failed; build aborted
INFO: Elapsed time: 1.565s
INFO: 0 processes.
FAILED: Build did NOT complete successfully (80 packages loaded, 588 targets configured)

[davido]

@c-parsons @laurentlb Can you clarify, why Bazel itself is not forward compatible and what is the right course of action here? How I supposed to fix Gerrit Code Review project and its transitive dependency projects, e.g. rules_closure, if Bazel itself is not compatible and build is aborting in Bazel code even before the breakages in Gerrit and rules_closure are reported?

  $ bazel version
  Build label: 0.24.0
  $ bazel build --incompatible_disallow_struct_provider_syntax :headless
INFO: Invocation ID: e51dfceb-c055-4970-8485-e689c8dca66c
ERROR: /home/davido/.cache/bazel/_bazel_davido/5c01f4f713b675540b8b424c5c647f63/external/bazel_tools/tools/jdk/BUILD:54:1: in java_toolchain_alias rule @bazel_tools//tools/jdk:current_java_toolchain: 


/home/davido/.cache/bazel/_bazel_davido/5c01f4f713b675540b8b424c5c647f63/external/bazel_tools/tools/jdk/java_toolchain_alias.bzl:85:12: Returning a struct from a rule implementation function is deprecated and will be removed soon. It may be temporarily re-enabled by setting --incompatible_disallow_struct_provider_syntax=false . See https://github.com/bazelbuild/bazel/issues/7347 for details.
ERROR: Analysis of target '//:headless' failed; build aborted: Analysis of target '@bazel_tools//tools/jdk:current_java_toolchain' failed; build aborted
INFO: Elapsed time: 0.156s
INFO: 0 processes.
FAILED: Build did NOT complete successfully (0 packages loaded, 0 targets configured)

Even on Bazel@HEAD (acc72bb) the relevant piece of the code is:

def _java_toolchain_alias(ctx):
    """An experimental implementation of java_toolchain_alias using toolchain resolution."""
    if java_common.is_java_toolchain_resolution_enabled_do_not_use(ctx = ctx):
        toolchain = ctx.toolchains["@bazel_tools//tools/jdk:toolchain_type"]
    else:
        toolchain = ctx.attr._java_toolchain[java_common.JavaToolchainInfo]
    return struct(
        providers = [toolchain],
        # Use the legacy provider syntax for compatibility with the native rules.
        java_toolchain = toolchain,
    )

[laurentlb]

It seems like this is not ready for migration. Removing the migration labels.

This change is breaking all downstream projects (https://buildkite.com/bazel/bazelisk-plus-incompatible-flags/builds/59), Bazel itself is not compatible.

[limdor]

What is the status of this topic? This is an issue with bazel at HEAD. If someone wants to use --all_incompatible_changes to be able to do migrations, then --incompatible_disallow_struct_provider_syntax gets also triggered and is failing to build because of the line in java_toolchain_alias. This can be workaround with --incompatible_disallow_struct_provider_syntax=false but it is not really a nice option.

The other option in case that this takes time to be fixed would be to remove --incompatible_disallow_struct_provider_syntax from --all_incompatible_changes, would that be an option?

[c-parsons]

Hello,

Apologies that this issue fell off my radar.

Re: Coverage:
https://docs.bazel.build/versions/master/skylark/rules.html#code-coverage-instrumentation
You should now be able to use this for modern coverage-related provider.

Re: java_toolchain_alias.bzl: I'll look into getting this fixed.

Re: incompatible flag, and --all_incompatible_changes, I would suggest you use the workaround you've mentioned, --all_incompatible_changes --incompatible_disallow_struct_provider_syntax=false, if you are blocked on the latter flag. It's not ideal, but we generally don't have infrastructure for removing --incompatible_ flags from the all_incompatible_changes set. The far better approach here would be to resolve any blockers of incompatible_disallow_struct_provider_syntax.

[brandjon]

Do I understand correctly that flipping this flag is sufficient to ensure that legacy struct providers are not used in a build? I.e., that you can't return "mystruct_provider" = struct(...) from a rule implementation function, and because of this, you also can't access mydep.mystruct_provider?

Presumably there might be some native rules that still have struct providers, and those would all have to be eliminated before we could delete the legacy code path for creating and reading structs in transitive info collections.

I'd love to be able to delete legacy structs altogether this year.

[brandjon]

Do I understand correctly that flipping this flag is sufficient to ensure that legacy struct providers are not used in a build?

Nm, just saw that #9014 is also needed.