Copy over runfiles library from Bazel

[fmeum]

This uses root_symlinks to preserve the fixed runfiles path at which the library must be available and the private skip_conflict_checking attribute to preserve backwards compatibility by not enabling strict runfiles path conflict checking.

[fmeum]

@meteorcloudy @Wyverald I'm curious to hear what you think about the strict conflict checking introduced by this PR.

[Wyverald]

I don't really understand the implication of this. Will we be removing the bash runfiles library from @bazel_tools? (I guess not completely, becuase it's hard to source the runfiles library from rules_shell.) Will we be leaving an alias behind?

Also what does "strict conflict checking" mean here?

[fmeum]

I don't really understand the implication of this. Will we be removing the bash runfiles library from @bazel_tools? (I guess not completely, becuase it's hard to source the runfiles library from rules_shell.) Will we be leaving an alias behind?

We only need to leave an alias behind for backwards compatibility.

Sourcing the runfiles library from rules_shell is possible with the exact same init code due to the trick with root symlinks (confirmed by the BCR test module).

Also what does "strict conflict checking" mean here?

Conflicts between runfiles (i.e. different runfiles mapping to the same rlocationpath) are usually silently ignored by Bazel, but as soon as symlinks or root symlinks are added via Starlark, they result in an error.

[Wyverald]

Thanks for the explanation!

Conflicts between runfiles (i.e. different runfiles mapping to the same rlocationpath) are usually silently ignored by Bazel, but as soon as symlinks or root symlinks are added via Starlark, they result in an error.

Is this behavior by design? Also, to confirm, regarding this part in your PR desciption:

Users will see an error if their runfiles trees contain both @bazel_tools//tools/bash/runfiles and @rules_shell//shell/runfiles with a Bazel version in which the former is not yet an alias to the latter. This ensures that conflicting versions can't be mixed.

there's nothing special about these two targets, right? Any conflict in any runfiles will cause an error, whereas before they would only do so if some other transitive dep specified [root] symlinks in their runfiles.

This PR LGTM given my very limited understanding, but I also kind of want @lberki to give a go-ahead.

[fmeum]

Yes, the strict check in the case of (root) symlinks has been an explicit choice: https://github.com/bazelbuild/bazel/blob/3ef2abcb3fa81618a93b6c21629f40867dc46528/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleContext.java#L1083

there's nothing special about these two targets, right? Any conflict in any runfiles will cause an error, whereas before they would only do so if some other transitive dep specified [root] symlinks in their runfiles.

That's correct. Although I just found the private skip_conflict_checking parameter of ctx.runfiles and I will use that for backwards compatibility.

[lberki]

I haven't read the pull request, but if that's the question, moving the runfiles library out of the Bazel repository is a fantastic idea. Do I need to engage further?

[Wyverald]

I haven't read the pull request, but if that's the question, moving the runfiles library out of the Bazel repository is a fantastic idea. Do I need to engage further?

The question was more about whether the new strict conflict checking is kosher, but @fmeum made the change to opt out of it so this PR is fully safe now.

Yes, the strict check in the case of (root) symlinks has been an explicit choice: https://github.com/bazelbuild/bazel/blob/3ef2abcb3fa81618a93b6c21629f40867dc46528/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleContext.java#L1083

I was curious and did a bit more digging, and apparently this has been there since at least 2018 (which is basically time immemorial): https://github.com/bazelbuild/bazel/blame/e24270826804ced8adb819e2078f472c8571b5df/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleContext.java#L1007