Starlark runfiles merging silently drops --legacy_external_runfiles external runfiles links

[rickeylev]

(Mostly filing this for posterity, as its a subtle behavior change that can be hard to debug)

Description of the bug:

When merging runfiles with the Starlark APIs (e.g. ctx.runfiles().merge() or ctx.runfiles().merge_all()), certain invocations will silently drop the "external/" paths that --legacy_external_runfiles (default true) creates.

ctx.runfiles(files) # keeps external
ctx.runfiles().merge(ctx.runfiles(files)) # keeps external
ctx.runfiles(files).merge(ctx.runfiles()) # keeps external

# The buggy call
ctx.runfiles(files).merge(ctx.runfiles(files)) #drops external

Looking at Runfiles.java, I see legacyExternalRunfiles is hard-coded to false in various spots.

I think the generalized case is: merging any two non-empty runfiles will drop legacyExternalRunfiles. This is because the Runfiles.Builder ctor argument legacyExternalRunfiles determines the final value (i.e. any intermediate
Runfiles objects being merged in don't have their legacyExternalRunfiles value considered).

The Runfiles.merge() logic is this:

def merge(this, other):
  if this is empty: return other
  if other is empty: return this

  builder = Builder(legacy_external_runfiles=False)  # We now have lost the setting
  builder = builder.merge(this)
  builder = builder.merge(other)
  return builder.build()

The Runfiles.mergeAll logic is harder to follow, but I think basically the same: all its builder calls force legacyExternalRunfiles=false.

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

# WORKSPACE
local_repository(
  name = "repo2",
  path = "repo2"
)
EOF

# repo2/BUILD
filegroup(name="r2files", srcs=["r2.txt"])

# BUILD
load(":foo.bzl", "foo_binary")
foo(name="foo", data=["@repo2//:r2files"])

# foo.bzl
def impl(ctx):
  files = ctx.files.data
  rf = ctx.runfiles(files).merge(ctx.runfiles(files)
  return [DefaultInfo(runfiles=rf)]

foo = rule(implementation=impl, attrs={"data": attr.label_list()})

# Run
bazel build //:foo
find bazel-bin/foo

Expected: the find command lists external/repo2/r2.txt

Actual: It does not

Which operating system are you running Bazel on?

Linux

What is the output of bazel info release?

dev

If bazel info release returns development version or (@non-git), tell us how you built Bazel.

bazel build //src:bazel-dev

What's the output of git remote get-url origin; git rev-parse master; git rev-parse HEAD ?

git@github.com:rickeylev/bazel.git
fd9bc99165104c41d19b16f3ef2ce8d48cbd330d
6172922d4f8f357393bdca435f7f0d4b5331dd51

Have you found anything relevant by searching the web?

flag flip for legacy_external_runfiles: #12821

Any other information, logs, or outputs that you want to share?

No response

[rickeylev]

Oh, I forgot add: IMHO this should just be left alone. The goal is to switch to --legacy_external_runfiles=false, right? This bug effectively means that Starlark code is effectively acting that way already -- in order to not trigger this bug, you have to never merge with another runfiles through Starlark.

It's probably just native rules being rewritten to Starlark that are affected, since they were able to use Java APIs that propagated legacy_external_runfiles.

[comius]

P4, I share the opinion to leave this alone :)