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): phst/runfiles#3 (comment)

Close #3036
Close #2359
Close #2422
Close #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🤔

[sluongng]

Small updates:

After several iteration in #3209 I could not figure out how to make this windows compatible. I don't have a local window environment to work with. In addition to that, some of the window behaviors was not clearly defined in java/python runfiles and the related test suites, which I was using as references for this Go's implementation.

https://github.com/bazelbuild/bazel/blob/884156ffc35eccb9ab56c849817cbf045986c968/tools/java/runfiles/testing/RunfilesTest.java#L246-L263

I would appreciated if somebody could help me test and potentially fix to make this work on windows. 🙏

Here are the changes from my latest iterations:

• Adjusted how 'canonical' path is evaluated. This could potentially deviate from the litterature in the GDoc proposal, but make things more consistent vs the native runfiles libs in @bazel_tools. Most notable change is that .. and . are accepted under current spec.

• Beefed up the test suite by including Bash's runfiles as the external data dependency. This should help highlighted how Windows's result could be quite inconsistent to work with.

• Temporarily disabled TestFS on Window. Not yet sure what to do with ExampleRunfiles test.

• Replaced all usages of path APIs, which is unix specific, in favor of path/filepath.

• Minor costmetic changes in hope that it helps improve readability

[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 #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 #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 bazelbuild/bazel#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 <#3205 (comment)>, 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.

[linzhp]

minor suggestion. LGTM

[linzhp]

Suggested change

	func (r *Runfiles) Path(s string) (string, error) {
	func (r *Runfiles) Path(relPath string) (string, error) {

[fmeum]

The function allows absolute paths as well, so I am going to use path instead and adding an example.

[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!