Runfiles should be relative to runfiles root, not workspace subdirectories

[phst]

Currently runfiles.go searches the workspace subdirectories for runfiles (https://github.com/bazelbuild/rules_go/blob/master/go/tools/bazel/runfiles.go#L58). That's inconsistent with all other runfile implementations I'm aware of, and shouldn't be needed, as users are in practice always aware of the workspace they want to use. I think this functionality should be removed, i.e. runfiles should always be relative to the runfiles root, like in the other languages.

This would be an incompatible change, so probably this should be implemented as a new function/library, with the current ones deprecated.

[phst]

If you agree with this bug and the others about runfiles, then I'm happy to contribute a new runfiles library.

[jayconrod]

Could you fill out the issue template and describe a situation where this doesn't work? Just saying it's inconsistent isn't enough to go on. I'm not sure any of the rules have consistent implementations for this. I don't think there are documented guidelines for how this is supposed to work either.

Getting runfiles to work at all with Bazel has been a pretty bad experience. The behavior is different:

• On different versions of Bazel.
• On different operating systems (Windows vs. others).
• With different commands (bazel run, bazel test, other actions executed by Bazel, binaries run outside Bazel).
• In different workspaces (main workspace, external workspace, main workspace when called as if it were an external workspace).

go/tools/bazel attempts to work in all those situations, though I'm sure there are cases it doesn't work. It would be better to fix those in place. I'm not willing to maintain another library though or make significant breaking changes to this one though.

[phst]

Could you fill out the issue template and describe a situation where this doesn't work?

Sure, here's an example (on macOS, with Bazel 2.0).

WORKSPACE file:

# Copyright 2020 Google LLC.
# SPDX-License-Identifier: Apache-2.0
load("@bazel_tools//tools/build_defs/repo:http.bzl", "http_archive")

http_archive(
    name = "io_bazel_rules_go",
    sha256 = "db2b2d35293f405430f553bc7a865a8749a8ef60c30287e90d2b278c32771afe",
    urls = [
        "https://mirror.bazel.build/github.com/bazelbuild/rules_go/releases/download/v0.22.3/rules_go-v0.22.3.tar.gz",
        "https://github.com/bazelbuild/rules_go/releases/download/v0.22.3/rules_go-v0.22.3.tar.gz",
    ],
)

load("@io_bazel_rules_go//go:deps.bzl", "go_register_toolchains", "go_rules_dependencies")

go_rules_dependencies()

go_register_toolchains()

new_local_repository(
    name = "other_workspace",
    build_file_content = 'exports_files(["WORKSPACE"])',
    path = "/",
    workspace_file_content = 'workspace(name = "other_workspace")',
)

BUILD file:

# Copyright 2020 Google LLC.
# SPDX-License-Identifier: Apache-2.0
load("@io_bazel_rules_go//go:def.bzl", "go_binary", "go_library", "go_test")

go_test(
    name = "runfile_clash_test",
    srcs = ["runfile_clash_test.go"],
    data = [
        "@io_bazel_rules_go//:all_files",
        "@other_workspace//:WORKSPACE",
    ],
    deps = ["@io_bazel_rules_go//go/tools/bazel:go_default_library"],
)

Test file (runfile_clash_test.go):

// Copyright 2020 Google LLC.
// SPDX-License-Identifier: Apache-2.0
package runfile_clash_test

import (
	"io/ioutil"
	"testing"

	"github.com/bazelbuild/rules_go/go/tools/bazel"
)

func TestRunfileClash(t *testing.T) {
	fn, err := bazel.Runfile("WORKSPACE")
	if err != nil {
		t.Fatal(err)
	}
	b, err := ioutil.ReadFile(fn)
	if err != nil {
		t.Fatal(err)
	}
	got := string(b)
	const want = `workspace(name = "other_workspace")`
	if got != want {
		t.Errorf("got %q, want %q", got, want)
	}
}

This fails because runfiles.go finds the io_bazel_rules_go workspace before the other_workspace workspace.

More generally, I was at first very surprised by the behavior inconsistency. AFAIK all other languages I've tried treat runfile names as relative to the runfile root.

Just saying it's inconsistent isn't enough to go on. I'm not sure any of the rules have consistent implementations for this.

I certainly haven't checked every other language, but the ones I did check are very consistent with each other. All libraries export a Rlocation function that requires the workspace name.

• C++: https://github.com/bazelbuild/bazel/blob/master/tools/cpp/runfiles/runfiles_src.h#L48
• Python: https://github.com/bazelbuild/bazel/blob/master/tools/python/runfiles/runfiles.py#L34
• Java: https://github.com/bazelbuild/bazel/blob/master/tools/java/runfiles/Runfiles.java#L53
• Bash: https://github.com/bazelbuild/bazel/blob/master/tools/bash/runfiles/runfiles.bash#L68
• Haskell: http://hackage.haskell.org/package/bazel-runfiles-0.7.0.1/docs/Bazel-Runfiles.html

Note that the API is really the same in all cases: there's a Runfiles class/type that first needs to be created (except for Bash, which doesn't have custom types), and that has a rlocation function/method requiring the filename relative to the runfiles root, including the workspace name. So I think these libraries are all pretty consistent with each other (even across vastly different languages such as Bash and Haskell), and the Go library is quite inconsistent with the rest.

I don't think there are documented guidelines for how this is supposed to work either.

That's true, but the C++ implementation is in practice a de facto standard.

Getting runfiles to work at all with Bazel has been a pretty bad experience.

I can see that this is a difficult topic given the lack of documentation and the various edge cases, but then I wonder why a literal translation of the C++ library wouldn't do the job just as well, and resolve these inconsistencies.

It would be better to fix those in place.

That's how I started when trying to use the current library, but I soon realized that its behavior is so vastly different from the other changes that keeping the current interface is probably harder than creating a new library from scratch.

I'm not willing to maintain another library though or make significant breaking changes to this one though.

I agree that there shouldn't be any breaking changes. However, I'd still argue that deprecating and freezing the current library and creating a new one (by translating the C++ one) would be a realistic approach, given that then only the new library would need to be maintained (and it could be quite a bit simpler, given that the handling of multiple workspaces is one of the more complex parts of the current library).

[phst]

https://docs.google.com/document/d/e/2PACX-1vSDIrFnFvEYhKsCMdGdD40wZRBX3m3aZ5HhVj4CtHPmiXKDCxioTUbYsDydjKtFDAzER5eg7OjJWs3V/pub (linked from https://github.com/bazelbuild/bazel/issues/10022) contains at least some information about the expected interface for runfiles libraries. While this is only directly geared towards the native rules, it's probably a good blueprint for other languages as well:

Every language's library will have a similar interface:

• a Create method that inspects the environment and/or argv[0] to determine the runfiles strategy (manifest-based or directory-based; see below), initializes runfiles handling and returns a Runfiles object
• an Rlocation(string) method that expects a runfiles-root-relative path [...] and returns the absolute path of the file, which is normalized [...] and uses "/" as directory separator on every platform (including Windows)

Emphasis mine; the emphasized parts highlight the differences to the current Go library.

[jayconrod]

Thanks for explaining. The example, links to other language implementations, and documentation are all very helpful.

I'm going to mark this as a bug for now, and I suspect that you're right that this library should be reimplemented.

[ashi009]

Coupling with #2423, probably it's time to create a runfiles package instead?

Is it possible to move the golang implemenation to https://github.com/bazelbuild/bazel/tree/master/tools?

[phst]

BTW, I've created such a package (https://pkg.go.dev/github.com/phst/runfiles), and I'd be happy to contribute it to this repository.

[phst]

Is it possible to move the golang implemenation to https://github.com/bazelbuild/bazel/tree/master/tools?

I don't think that's technically feasible because AIUI it would require Bazel core to depend on rules_go, which would introduce a cyclic dependency.

[sluongng]

@fmeum @phst I think there is value in upstreaming the runfiles lib into rules_go

Has there been any attempt in doing this? Any blocker?

If there has been no attempt, I could give it a shot in the near future. Roughly something like this:

• Cleanup runfiles.go from deprecated interfaces
• Port the new implementation over
• Mark old APIs as deprecated
• Slowly replacing old calls with new ones in the remaining exported func

[fmeum]

@sluongng That would be highly appreciated.

Just keep in mind that there is https://github.com/bazelbuild/bazel/issues/15029, meaning that all runfiles libraries will potentially have to undergo fundamental changes before Bazel 6. For Go, this might require some unusual tricks to offer a good API.

[sluongng]

Ok let me put together some PRs and send them upstream.

Let's see if I can move faster than Bazel 6 👀