Always collect FileProvider's filesToBuild as data runfiles

[fmeum]

Guidelines for Starlark rules suggest that data-like attributes on
rules should always merge the default outputs of rule targets into the
transitive runfiles. See:
https://docs.bazel.build/versions/main/skylark/rules.html#runfiles-features-to-avoid

As a result, Starlark rules generally don't (and shouldn't) explicitly
include their default outputs into their runfiles. Before this commit,
native rules did not merge these outputs in the same way as idiomatic
Starlark rules, which led to unexpectedly absent runfiles.

Fixes #15043

[comius]

This looks reasonable. Mind that we're rewriting all the rules where this applies to Starlark.

I'll run internal unit tests, to see if this breaks anything, but I don't expect it to.

[comius]

I'll run internal unit tests, to see if this breaks anything, but I don't expect it to.

There are some breakages on internal unit tests, but it seems just the tests have to be updated.

[fmeum]

This looks reasonable. Mind that we're rewriting all the rules where this applies to Starlark.

I'll run internal unit tests, to see if this breaks anything, but I don't expect it to.

Thanks!

[fmeum]

@comius Did you get a chance to look into the internal unit tests blocking this?

[comius]

@comius Did you get a chance to look into the internal unit tests blocking this?

I've put this on my backlog, I will need to do a bit more research and that will take more time.

What happens is that .ifso files from data dependency of a dynamic cc_binary (linkstatic=0) get added to runfiles, and there's tests, that verifies this doesn't happen.

And .so and .a gets added from data dep in the case of static cc_binary (linkstatic=1)

[fmeum]

@comius Is there anything I can do to help get this into 5.3.0?

The issue fixed by the PR is, based on my personal experience, one of the top gotchas for users writing their first custom rules (including myself and many of my coworkers at some point). It also frequently comes up on the Bazel Slack (most recently at https://bazelbuild.slack.com/archives/CA31HN1T3/p1657044541910759?thread_ts=1657044541.910759&cid=CA31HN1T3). Getting it fixed in Bazel 5 would also help rule authors ditch data_runfiles possibly 9 months earlier.

Would it help if I guarded the change behind a flag that you could flip back inside Google? Or, alternatively, can the failing tests be open-sourced? I could then try to fix the runfile pollution issues in the cc rules.

[fmeum]

@comius Friendly ping. This is still coming up on Bazel Slack over and over again as a major inconsistency.

Do you think we can get this into Bazel 6? It looks like Starlark cc_binary already has the fixed behavior, otherwise the test wouldn't pass. Presumably the internal tests have been fixed?

[comius]

I did the changes needed internally - and I'm waiting for feedback from reviewers.

[comius]

I have a lot of failures running this change through the depot, I still need to analyse them thought.

It makes sense to put this behind an incompatible flag - because others might have similar problems (and if I don't fix all the failures internally we can still release this in Bazel).

One bug that I already located in the run and you can probably fix from stack trace:

Caused by: java.lang.IllegalArgumentException: Order mismatch: preorder != postorder
	at com.google.common.base.Preconditions.checkArgument(Preconditions.java:441)
	at com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder.addTransitive(NestedSetBuilder.java:138)
	at com.google.devtools.build.lib.analysis.Runfiles$Builder.addTransitiveArtifacts(Runfiles.java:777)
	at com.google.devtools.build.lib.analysis.Runfiles$Builder.addTargetIncludingFileTargets(Runfiles.java:988)
	at com.google.devtools.build.lib.analysis.Runfiles$Builder.addTarget(Runfiles.java:964)
	at com.google.devtools.build.lib.analysis.Runfiles$Builder.addTargets(Runfiles.java:957)
	at com.google.devtools.build.lib.analysis.Runfiles$Builder.addDataDeps(Runfiles.java:938)
	at com.google.devtools.build.lib.analysis.Runfiles$Builder.addRunfiles(Runfiles.java:910)
	at com.google.devtools.build.lib.view.sh.ShBinary.create(ShBinary.java:71)
	at com.google.devtools.build.lib.view.sh.ShBinary.create(ShBinary.java:30)
	at com.google.devtools.build.lib.analysis.ConfiguredTargetFactory.createRule(ConfiguredTargetFactory.java:371)

[fmeum]

I'm actually quite confused by the stack trace: I expected both filesToBuild and runfiles to use stable order nested sets, but the exception clearly shows that neither are using the stable order in this case. Is that possible and something that the code should be able to handle?

[comius]

I'm actually quite confused by the stack trace: I expected both filesToBuild and runfiles to use stable order nested sets, but the exception clearly shows that neither are using the stable order in this case. Is that possible and something that the code should be able to handle?

You can rewrap NestedSet to a stable order which will prevent the exception. Example is Runfiles.addTransitiveArtifactsWrappedInStableOrder

[fmeum]

@comius @meteorcloudy Sorry, forgot to update the tests. Should be fixed now.

[meteorcloudy]

@fmeum Can you follow https://bazel.build/contribute/breaking-changes to file a tracking issue for this incompatible change?

Once this is merged, I'll add the correct labels and it'll be tested in the incompatible flag pipeline

[fmeum]

@meteorcloudy I created #16654.

[fmeum]

@bazel-io flag

[fmeum]

@meteorcloudy This has been merged, could you add it to the pipeline?

[meteorcloudy]

@bazel-io fork 6.0.0

[meteorcloudy]

@fmeum This flag is already tested in https://buildkite.com/bazel/bazelisk-plus-incompatible-flags/builds/1325, currently failing with "Unrecognized option" error, we'll get some real result tomorrow ;)