TestOSExitNotAllowed fails when `go test` is passed `-trimpath`

Foxboron commented 5 months ago

Checklist

[X] I have read through the manual page (man fzf)
[X] I have searched through the existing issues
[X] For bug reports, I have checked if the bug is reproducible in the latest version of fzf

Output of `fzf --version`

N/A

OS

[X] Linux
[ ] macOS
[ ] Windows
[ ] Etc.

Shell

[X] bash
[ ] zsh
[ ] fish

Problem / Steps to reproduce

λ fzf master Ɇ » go test ./...
?       github.com/junegunn/fzf/src/protector   [no test files]
ok      github.com/junegunn/fzf 1.449s
ok      github.com/junegunn/fzf/src (cached)
ok      github.com/junegunn/fzf/src/algo    (cached)
ok      github.com/junegunn/fzf/src/tui (cached)
ok      github.com/junegunn/fzf/src/util    (cached)
λ fzf master Ɇ » go test -trimpath ./...
?       github.com/junegunn/fzf/src/protector   [no test files]
--- FAIL: TestOSExitNotAllowed (0.02s)
    --- FAIL: TestOSExitNotAllowed/github.com/junegunn/fzf (0.00s)
        main_test.go:91: /home/fox/Git/prosjekter/Go/fzf/main.go:4:4: could not import embed (cannot find package "embed" in any of:
                ($GOROOT not set)
                /home/fox/.go/src/embed (from $GOPATH))
    --- FAIL: TestOSExitNotAllowed/github.com/junegunn/fzf/src (0.01s)
        main_test.go:91: /home/fox/Git/prosjekter/Go/fzf/src/actiontype_string.go:5:8: could not import strconv (cannot find package "strconv" in any of:
                ($GOROOT not set)
                /home/fox/.go/src/strconv (from $GOPATH))
    --- FAIL: TestOSExitNotAllowed/github.com/junegunn/fzf/src/algo (0.00s)
        main_test.go:91: /home/fox/Git/prosjekter/Go/fzf/src/algo/algo.go:81:2: could not import bytes (cannot find package "bytes" in any of:
                ($GOROOT not set)
                /home/fox/.go/src/bytes (from $GOPATH))
    --- FAIL: TestOSExitNotAllowed/github.com/junegunn/fzf/src/tui (0.00s)
        main_test.go:91: /home/fox/Git/prosjekter/Go/fzf/src/tui/eventtype_string.go:5:8: could not import strconv (cannot find package "strconv" in any of:
                ($GOROOT not set)
                /home/fox/.go/src/strconv (from $GOPATH))
    --- FAIL: TestOSExitNotAllowed/github.com/junegunn/fzf/src/util (0.00s)
        main_test.go:91: /home/fox/Git/prosjekter/Go/fzf/src/util/atexit.go:4:2: could not import os (cannot find package "os" in any of:
                ($GOROOT not set)
                /home/fox/.go/src/os (from $GOPATH))
FAIL
FAIL    github.com/junegunn/fzf 0.020s
ok      github.com/junegunn/fzf/src (cached)
ok      github.com/junegunn/fzf/src/algo    (cached)
ok      github.com/junegunn/fzf/src/tui (cached)
ok      github.com/junegunn/fzf/src/util    (cached)
FAIL

Most of the relevant code appeared in this path https://github.com/junegunn/fzf/commit/3c877c504b6102daf5dcc1083b1f1a7db88d304c

junegunn commented 5 months ago

I can't reproduce the problem

$ go test -trimpath ./...
?       github.com/junegunn/fzf/src/protector   [no test files]
ok      github.com/junegunn/fzf (cached)
ok      github.com/junegunn/fzf/src     (cached)
ok      github.com/junegunn/fzf/src/algo        (cached)
ok      github.com/junegunn/fzf/src/tui (cached)
ok      github.com/junegunn/fzf/src/util        (cached)

$ go version
go version go1.20.13 darwin/arm64

$ echo $GOPATH
/Users/jg/gosrc

$ unset GOPATH

$ go test -trimpath ./...
go: downloading github.com/rivo/uniseg v0.4.7
go: downloading golang.org/x/sys v0.19.0
go: downloading golang.org/x/term v0.19.0
go: downloading github.com/mattn/go-isatty v0.0.20
go: downloading github.com/charlievieth/fastwalk v1.0.3
go: downloading github.com/mattn/go-shellwords v1.0.12
?       github.com/junegunn/fzf/src/protector   [no test files]
ok      github.com/junegunn/fzf 1.540s
ok      github.com/junegunn/fzf/src     0.524s
ok      github.com/junegunn/fzf/src/algo        1.058s
ok      github.com/junegunn/fzf/src/tui 0.833s
ok      github.com/junegunn/fzf/src/util        0.485s

Foxboron commented 5 months ago

Huh, how weird. I can reproduce this on my local system + my build container. I'll probably need to dig into this more to figure out what the problem is then.

junegunn commented 5 months ago

I tried again with a fresh copy, and it works just fine.

$ unset GOPATH
$ git clone https://github.com/junegunn/fzf.git
Cloning into 'fzf'...
remote: Enumerating objects: 14063, done.
remote: Counting objects: 100% (2603/2603), done.
remote: Compressing objects: 100% (390/390), done.
remote: Total 14063 (delta 2404), reused 2324 (delta 2201), pack-reused 11460
Receiving objects: 100% (14063/14063), 5.48 MiB | 24.09 MiB/s, done.
Resolving deltas: 100% (9062/9062), done.
$ cd fzf
$ go test -trimpath ./...
?       github.com/junegunn/fzf/src/protector   [no test files]
ok      github.com/junegunn/fzf 1.546s
ok      github.com/junegunn/fzf/src     (cached)
ok      github.com/junegunn/fzf/src/algo        (cached)
ok      github.com/junegunn/fzf/src/tui (cached)
ok      github.com/junegunn/fzf/src/util        (cached)
$ go version
go version go1.20.13 darwin/arm64

Foxboron commented 5 months ago

Is suspect you have GOROOT set? What does env say?

junegunn commented 5 months ago

I don't have it.

$ unset GOPATH
$ env | grep GO
$

By the way, what's the point of setting -trimpath when running go test?

Foxboron commented 5 months ago

By the way, what's the point of setting -trimpath when running go test?

We are setting a global GOFLAGS for the build which contains -trimpath.

junegunn commented 5 months ago

Reopening it as I can reproduce it on an Ubuntu 24.04 image with go 1.22.2.

junegunn commented 5 months ago

@charlievieth Hi, any idea why this test doesn't work on some systems?

junegunn commented 5 months ago

Reopening it as I can reproduce it on an Ubuntu 24.04 image with go 1.22.2.

Setting GOROOT fixed the issue:

GOROOT=$(go env GOROOT) go test -trimpath ./...

Foxboron commented 5 months ago

Right, but GOROOT is a new requirement so this is either something missing in the test (API breakage?) or something amiss with the library being used (regression?)

junegunn commented 5 months ago

I've no idea to be honest. The test code was written by @charlievieth and I'm not fully familiar with the libraries used in it. Considering that it works without GOROOT on macOS, maybe it's a bug in the Go distribution or the Linux package?

Anyway, this GOROOT requirement doesn't apply to build command, and it actually makes more sense to not use -trimpath in test context (see https://github.com/golang/vscode-go/issues/2737), I think it's fine.

charlievieth commented 5 months ago

Thank you for flaggifng this and I'll take a look at fixing this with -trimpath when I have some cycles but that might be a few days. Basically, the goal of this test is to ensure that os.Exit is not called, I could look at how golang.org/x/tools/go/analysis works since it performs similar analysis and see if it handles -trimpath being set.

That said, I'm not sure -trimpath provides much benefit when running tests - IIRC it's mostly for distributing builds.

Foxboron commented 5 months ago

Thank you for flaggifng this and I'll take a look at fixing this with -trimpath when I have some cycles but that might be a few days.

Thanks for looking at it. I tried looking at the code initially but didn't get very far.

That said, I'm not sure -trimpath provides much benefit when running tests - IIRC it's mostly for distributing builds.

I don't disagree on this, but build systems in distros sets a global GOFLAGS variable and having to replace out -trimpath because the test fails when it's set is not optimal.

junegunn commented 5 months ago

build systems in distros sets a global GOFLAGS variable and having to replace out -trimpath

How about setting GOROOT as well instead of removing -trimpath? Seems like an easier and safer approach.

Foxboron commented 5 months ago

GOROOT=$(go env GOROOT) is inherently redundant, I'm curious why this has regressed.

junegunn commented 5 months ago

A quick research.

build.ImportDir refers to runtime.GOROOT(),

https://github.com/golang/go/blob/16ce8b3925deaeb72541ee96b6ee23a08fc21dea/src/go/build/build.go#L310-L317

and this is the definition of runtime.GOROOT():

But go env seems to do more work to than just referring to it to locate the real GOROOT.

https://github.com/golang/go/blob/16ce8b3925deaeb72541ee96b6ee23a08fc21dea/src/cmd/go/internal/cfg/cfg.go#L472

So that I think explains why the test requires GOROOT. So until they clear up the differences, GOROOT=$(go env GOROOT) is the only option. It may seem redundant, but it isn't for now, and it's safe and future-proof.

charlievieth commented 5 months ago

Created https://github.com/junegunn/fzf/pull/3758 to resolve this issue. Testing it locally with go test -trimpath works on my mac.

junegunn / fzf