Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cmd/go: set module root directory so editors/scripts see filenames in error messages relative to current directory #47399

Open
jwatte opened this issue Jul 26, 2021 · 8 comments

Comments

@jwatte
Copy link

@jwatte jwatte commented Jul 26, 2021

What version of Go are you using (go version)?

$ go version
go version go1.16.6 linux/amd64

Does this issue reproduce with the latest release?

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/jwatte/.cache/go-build"
GOENV="/home/jwatte/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/jwatte/observe/code/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/jwatte/observe/code/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="go1.16.6"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/dev/null"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build1083558025=/tmp/go-build -gno-record-gcc-switches"

What did you do?

First: This issue is created based on the discussion of #47363 but given that that issue was closed without any resolution to the underlying problem, as it didn't make the problem clear enough, here is the refined issue.

What did you see?

This issue is a "user story" (in the sense of "a legitimate user need that is currently not fulfilled") and could be solved in more than one way. Abbreviating from the conclusion, the behavior "the working directory determines the module" coupled with the behavior "diagnostic file names are printed relative to the working directory" breaks a lot of existing tooling in real-world projects.

  • If go build|run|test ./path/to/directory used the indicated directory as the root from which to discover which module it's in
  • If the go command had an option --module-path that overrode what the current directory was when determining which module it's in
  • If there existed an option --print-full-paths that made go tooling print full paths to files in error/failure messages, rather than relative to the current working directory

What did you expect to see?

The problem is one of "I'm a user of go, Typescript, C++, protobuf, and a number of other languages, that all live in a single monorepo, and I want editors to be able to build and test code, and open the correct file when it fails, no matter what particular language is involved in the failure."

One of the subdirectories of this repo contains go code. The root of this monorepo is not a go module. I may not even be focused on a go module when I want to build integration tests.

The working directory of the editor (be it vim, emacs, vscode, or something else) is the root of the monorepo, which is not inside the go module.

Build and test commands invoked by the editor end up being run from the current working directory of the editor, which is the root of the monorepo ;they provide the relative path of the file in question from the editor working directory as argument. This works fine for all the other languages we use.

Currently, I can't find a good way of gluing this presumably not uncommon setup into the go build system, such that the error parser of each kind of editor (vim, emacs, vscode, and more) will find and open the correct file for a test failure (or build failure.)

For the tooling and editors we use, the following things are true:

  • Working directory is the root of the monorepo, e g /home/me/checkout
  • Builds/tests may span more than one language/module, and do something like make -C cpp/path/module that's appropriate for each language
  • Errors are printed relative to the CWD, thus an error in a cpp file will say subdir/path/module/include/someheader.h:123: template is too complex, or errors are printed with full paths to filenames.
  • All existing tooling and editors work great in building and testing, and they find the file to open

go builds used to do go test ./go/src/whatever/package and that used to work (and with GO111MODULE still works)
With GO111MODULE turned off, that invocation now errors out, complaining that the CWD is not within a module, even though the interesting directory in question is within a module.

For purposes of this comment, let's consider an overall project that looks like:

/home/me/mything:
  cpp/
  docs/
  go/
    src/
      whatever/
        go.mod
        vendor/
        package/
          some_test.go
  haskell/
  typescript/
  react/

If I were to change the build command for the go package to do something like cd go/src/whatever && go test ./package then the filename printed will be package/myfile_test.go:123: insufficient shoe size which is relative to go/src/whatever, not relative to the editor CWD, and thus the editor won't find and open the file.

Best would be "if a directory is the argument to the command, then determine the module based on that directory, not based on the CWD" which would make the current setup Just Work.

Second best would be "allow the module-to-assume to be specified using a separate command line option" which means we could specify the go build/test command as go test --module-root=./go/src/whatever whatever/package -- this is a little more cumbersome because of the duplication of "whatever" in the path (it's both the root of the module, and the root package name) but it can be made to work.

Third best would be a command line option that forces the tooling to always output full path filenames. This will let the editor find the right file no matter what the working directory is. We'd invoke it like cd go/src/whatever && go test --print-full-paths whatever/package

@jayconrod jayconrod changed the title Editors/scripts finding the failing file:line from errors/failures fails in monorepo projects when GO111MODULE is disabled cmd/go: set module root directory so editors/scripts see filenames in error messages relative to current directory Jul 26, 2021
@jayconrod jayconrod added this to the Backlog milestone Jul 26, 2021
@rsc
Copy link
Contributor

@rsc rsc commented Jul 26, 2021

Best would be "if a directory is the argument to the command, then determine the module based on that directory, not based on the CWD" which would make the current setup Just Work.

And would break other setups that assume paths are relative to the current directory.
If you want paths relative to ./package, why not cd ./package && go test?

@jayconrod
Copy link
Contributor

@jayconrod jayconrod commented Jul 26, 2021

@jwatte Thanks for writing this up. I think I have a better understanding of the problem you're experiencing now.

A couple ideas for possible solutions:

  • go env GOMOD shows the main module's go.mod file. Unlike most other variables, this one cannot be set in the environment. If we let GOMOD be set explicitly, module-aware commands could use that instead of the working directory. This is similar to your --module-path suggestion, just reusing an existing name.
  • We could add a flag, say -effectivedir, to cmd/go setting the effective working directory. Command line arguments would still be interpreted relative to the actual working directory, but -effectivedir would be used to discover go.mod and go.work files (#45713).
  • For having the compiler show absolute paths, the compiler already accepts an -L flag with the description show full file names in error messages. It doesn't seem to actually do that though, so I may be misinterpreting its intent.

Agreed with @rsc above though, we cannot change what go build ./path/to/directory does; it's too big of a breaking change.

cc @bcmills @matloob

@jwatte
Copy link
Author

@jwatte jwatte commented Jul 26, 2021

If you want paths relative to ./package, why not cd ./package && go test?

That's not what I want. I want paths printed in error messages to be relative to the working directory of the invoking script/editor, which is not the root or sub-path of the module, for hopefully good reasons explained above.

Any one of the proposals enumerated by @jayconrod would allow me to solve the problems I'm seeing.

@bcmills
Copy link
Member

@bcmills bcmills commented Aug 3, 2021

Given #45713, perhaps it would be reasonable for go test ./go/src/whatever/package to succeed provided that the module in ./go/src/whatever is included in the current workspace? (go test $PWD/go/src/whatever/pkg certainly ought to work in that case.)

Then you could keep invoking most go commands in the workspace root (rather than the module root) and get paths relative to the workspace that way. (However, as currently proposed in #45713 that wouldn't work for commands like go get and go mod tidy that apply to a specific module rather than the whole workspace.)

@bcmills
Copy link
Member

@bcmills bcmills commented Aug 3, 2021

If we let GOMOD be set explicitly, module-aware commands could use that instead of the working directory.

I like that idea. I think I may have suggested it myself in the past a couple of times. 😉

@bcmills
Copy link
Member

@bcmills bcmills commented Aug 3, 2021

If we allow GOMOD to be set, I think we would still need ./… to be relative to the actual working directory (and to error out if the working directory is not within the module and/or workspace). But it would be fine to accept commands that combine absolute paths and patterns, like

GOMOD=$(pwd)/go/src/go.mod go test $GOMOD/...

@rsc
Copy link
Contributor

@rsc rsc commented Aug 3, 2021

For fixing error messages, let's not make very big changes.
Instead, let's add a build flag -abspath that means print all the errors in absolute paths.
The implementation would be an early exit in base.ShortPath.

@jwatte
Copy link
Author

@jwatte jwatte commented Aug 10, 2021

is included in the current workspace

If by "workspace" you mean "arbitrary file system directory from git/perforce/mercurial/tarballs" where the "workspace" has nothing to do with go itself (other than containing some go code/module in some possibly deep subdirectory) then, yes, that would be one of the options that would work.

Instead, let's add a build flag -abspath that means print all the errors in absolute paths.

That would also work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants