runfiles: port phst/runfiles to rules_go

[sluongng]

Today rules_go provide //go/tools/bazel as the canonical runfiles library for binaries to be executed with bazel test and bazel run.

However, the current implementation pre-date the recent changes in Bazel's upstream. Since then, all of the native runfiles library of Bash, Java, CPP, Python have been refactored to follow a certain convention in locating files. (1)

Although these are subjected to change with the incoming BzlMod feature, it would be easier to maintain if we can keep rules_go's runfiles library implementation aligned with native languages' implementation.

Today, it seems like https://github.com/phst/runfiles implemented exactly that. So with @fmeum suggestion and @phst permission (2), let's port the newer, more accurate implementation to rules_go.

Future refactoring will mark the current exported APIs in //go/tools/bazel as deprecated and/or swapping out the old implementation underneath to use this newer package.

Changes in this PR included:

• Copy paste repository over
• Removal of .git and .gitignore and .githooks dir
• Removal of repository specific files: README, WORKSPACE, LICENSE, CONTRIBUTE etc...
• Rename BUILD to BUILD.bazel
• Rename import path for both go and BUILD files
• Run gazelle over the packages
• Adjusted test cases to reflect new package paths
• Removed godoc related to installation instruction

(1): https://docs.google.com/document/d/e/2PACX-1vSDIrFnFvEYhKsCMdGdD40wZRBX3m3aZ5HhVj4CtHPmiXKDCxioTUbYsDydjKtFDAzER5eg7OjJWs3V/pub (2): https://github.com/phst/runfiles/issues/3#issuecomment-1003070026

Close #3036 #2359 #2422 #2423

[sluongng]

cc: @fmeum and @phst 🙏

[sluongng]

Executing tests from //go/tools/bazel/runfiles:runfiles_test
-----------------------------------------------------------------------------
--- FAIL: TestFS (0.00s)
    fs_test.go:41: TestFS found errors:
        .: Open: open .: file does not exist
        expected but not found: io_bazel_rules_go/go/tools/bazel/runfiles/test.txt
        expected but not found: io_bazel_rules_go/go/tools/bazel/runfiles/testprog/testprog
--- FAIL: TestRunfiles_manifestWithDir (0.00s)
    --- FAIL: TestRunfiles_manifestWithDir/foo/dir (0.00s)
        runfiles_test.go:126: Path failed: got "path\\to\\foo\\dir", want "path/to/foo/dir"
    --- FAIL: TestRunfiles_manifestWithDir/foo/dir/file (0.00s)
        runfiles_test.go:126: Path failed: got "path\\to\\foo\\dir/file", want "path/to/foo/dir/file"
    --- FAIL: TestRunfiles_manifestWithDir/foo/dir/deeply/nested/file (0.00s)
        runfiles_test.go:126: Path failed: got "path\\to\\foo\\dir/deeply/nested/file", want "path/to/foo/dir/deeply/nested/file"
--- FAIL: ExampleRunfiles (0.00s)
panic: exec: "C:\\b\\osvue4qe\\execroot\\io_bazel_rules_go\\bazel-out\\x64_windows-fastbuild\\bin\\go\\tools\\bazel\\runfiles\\testprog\\testprog": file does not exist [recovered]
	panic: exec: "C:\\b\\osvue4qe\\execroot\\io_bazel_rules_go\\bazel-out\\x64_windows-fastbuild\\bin\\go\\tools\\bazel\\runfiles\\testprog\\testprog": file does not exist

goroutine 1 [running]:
testing.(*InternalExample).processRunResult(0xc0000c7c70, {0x0, 0x0}, 0xc00007aae0, 0x0, {0x56f340, 0xc0001060e0})
	GOROOT/src/testing/example.go:91 +0x505
testing.runExample.func2()
	GOROOT/src/testing/run_example.go:60 +0x11c
panic({0x56f340, 0xc0001060e0})
	GOROOT/src/runtime/panic.go:1038 +0x215
go/tools/bazel/runfiles/runfiles_test_test.ExampleRunfiles()
	go/tools/bazel/runfiles/runfiles_test.go:59 +0x1d3
testing.runExample({{0x592d02, 0xf}, 0x59ed70, {0x5903e4, 0x4}, 0x0})
	GOROOT/src/testing/run_example.go:64 +0x28d
testing.runExamples(0xc0000c7e38, {0x6bf0a0, 0x2, 0x6})
	GOROOT/src/testing/example.go:44 +0x18e
testing.(*M).Run(0xc0000ec180)
	GOROOT/src/testing/testing.go:1505 +0x587
main.main()
	bazel-out/x64_windows-fastbuild/bin/go/tools/bazel/runfiles/runfiles_test_/testmain.go:110 +0x2b4

Ah window 🙃

[sluongng]

> git diff HEAD@{2}..HEAD
diff --git a/go/tools/bazel/runfiles/runfiles_test.go b/go/tools/bazel/runfiles/runfiles_test.go
index 610ca79f..78fb1ce7 100644
--- a/go/tools/bazel/runfiles/runfiles_test.go
+++ b/go/tools/bazel/runfiles/runfiles_test.go
@@ -47,7 +47,11 @@ func ExampleRunfiles() {
        }
        // The binary “testprog” is itself built with Bazel, and needs
        // runfiles.
-       prog, err := r.Path("io_bazel_rules_go/go/tools/bazel/runfiles/testprog/testprog")
+       rlocation := "io_bazel_rules_go/go/tools/bazel/runfiles/testprog/testprog"
+       if runtime.GOOS == "windows" {
+               rlocation = strings.ReplaceAll(rlocation, "/", "\\")
+       }
+       prog, err := r.Path(rlocation)
        if err != nil {
                panic(err)
        }
@@ -121,6 +125,7 @@ func TestRunfiles_manifestWithDir(t *testing.T) {
        } {
                t.Run(rlocation, func(t *testing.T) {
                        if runtime.GOOS == "windows" {
+                               rlocation = strings.ReplaceAll(rlocation, "/", "\\")
                                want = strings.ReplaceAll(want, "/", "\\")
                        }
                        got, err := r.Path(rlocation)

Tried some cheap trick but it does not seems to work. It seems like we need to implement window logic properly into the Runfiles struct.

I will have to re-visit this later 🤔

[fmeum]

Thanks for working on this, let me know if I can help.

Just one comment for now: It would be great if we could make the runfiles library available at the canonical location as per https://docs.bazel.build/versions/main/skylark/deploying.html#runfiles-library, even if only via an alias.

[sluongng]

Thanks for working on this, let me know if I can help.

Just one comment for now: It would be great if we could make the runfiles library available at the canonical location as per https://docs.bazel.build/versions/main/skylark/deploying.html#runfiles-library, even if only via an alias.

Yeah I think alias would be the way to do it.

That might be 2-3 PR into the future though. As I noted in the description, I plan to visit how the current API work after this and refactor / deprecate things where possible.

[sluongng]

worth to note that I do not have a window machine available to dev... so it would take me a bit of time to iterate on this.

I will create a draft PR to test it against window CI of this repo to avoid notifications to others🤔

[fmeum]

Thanks for digging into this! I should be able to take over and debug the Windows issues in the coming weeks.

[sluongng]

@fmeum Did you have a chance to review what is the expectations for runfiles lib on window?

[fmeum]

@sluongng I'm trying some things over at https://github.com/bazelbuild/rules_go/pull/3253. Looks like I got something wrong in my initial implementation of "runfiles in rundirs" lookup, sorry for that.

[fmeum]

@sluongng Sorry for the long wait, Bzlmod ate up most of my "Bazel budget".

Tests should be passing over at https://github.com/bazelbuild/rules_go/pull/3253 now. Feel free to take my two commits and squash them into yours.

Having the runfiles library upstream will also make it easier to implement https://github.com/bazelbuild/bazel/issues/16124 for Go, which I'm going to work on next.

[fmeum]

@sluongng If you are busy, would you be okay with me squashing my changes into your branch for this PR and merging?

[sluongng]

Yes please go ahead. I am a bit occupied for the next 2 months on non-bazel stuffs 🙈

On Tue, Nov 1, 2022, 9:55 AM Fabian Meumertzheim @.***> wrote:

@sluongng https://github.com/sluongng If you are busy, would you be okay with me squashing my changes into your branch for this PR and merging?

— Reply to this email directly, view it on GitHub https://github.com/bazelbuild/rules_go/pull/3205#issuecomment-1298231254, or unsubscribe https://github.com/notifications/unsubscribe-auth/AGLSXGIM47JC5QSNXQUJDELWGDLJ3ANCNFSM5ZBRTYMA . You are receiving this because you were mentioned.Message ID: @.***>

[fmeum]

@linzhp @achew22 I reviewed the code and fixed the tests. Since is a rather large PR and I have authored some parts, I would like to get an approval from one of you before merging.

[fmeum]

I also added a deprecation comment to the old runfiles function. It's package pro ides other functions for which we don't have full replacements yet, but that particular one can be fully replaced.

[fmeum]

Thanks again, @sluongng!