Skip to content

x/tools/go/packages: clarify naming convention of overlay files #67541

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.

Sign up for GitHub

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

Jump to bottom
Open
fxrlv opened this issue May 21, 2024 · 4 comments
Open

x/tools/go/packages: clarify naming convention of overlay files #67541

fxrlv opened this issue May 21, 2024 · 4 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone
Unreleased

Comments

@fxrlv
Copy link
Contributor

fxrlv commented May 21, 2024

Go version

go version go1.22.3 darwin/arm64

Output of go env in your module/workspace:

GO111MODULE=''
GOARCH='arm64'
GOBIN=''
GOCACHE='/Users/fxrlv/Library/Caches/go-build'
GOENV='/Users/fxrlv/Library/Application Support/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='arm64'
GOHOSTOS='darwin'
GOINSECURE=''
GOMODCACHE='/Users/fxrlv/go/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='darwin'
GOPATH='/Users/fxrlv/go'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/opt/homebrew/Cellar/go/1.22.3/libexec'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/opt/homebrew/Cellar/go/1.22.3/libexec/pkg/tool/darwin_arm64'
GOVCS=''
GOVERSION='go1.22.3'
GCCGO='gccgo'
AR='ar'
CC='cc'
CXX='c++'
CGO_ENABLED='1'
GOMOD='/dev/null'
GOWORK=''
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
PKG_CONFIG='pkg-config'
GOGCCFLAGS='-fPIC -arch arm64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -ffile-prefix-map=/var/folders/mg/1l0261p97kzflrmx68mm4z7c0000gn/T/go-build1456754968=/tmp/go-build -gno-record-gcc-switches -fno-common'

What did you do?

https://pkg.go.dev/golang.org/x/tools/go/packages#DriverRequest

// Overlay maps file paths (relative to the driver's working directory) to the byte contents
// of overlay files.
Overlay map[string][]byte `json:"overlay"`

https://pkg.go.dev/golang.org/x/tools/go/packages#Config

// Overlay provides a mapping of absolute file paths to file contents.
// If the file with the given path already exists, the parser will use the
// alternative file contents provided by the map.
//
// Overlays provide incomplete support for when a given file doesn't
// already exist on disk. See the package doc above for more details.
Overlay map[string][]byte

The DriverRequest's documentation states that paths are relative meanwhile Config's documentation states the opposite.
There is no problem here but little inconsistency.

What did you see happen?

The next driver invocation that happens inside packages.Load(...) totally violates the path convention the documentation set.

https://github.com/golang/tools/blob/c3aae998cf1d05bd3465e576730c67a9df71b4fa/go/packages/external.go#L112

req, err := json.Marshal(DriverRequest{
	// ...
	Overlay:    cfg.Overlay,
})

cfg.Overlay is not changed anywhere before.
So on the left side there should be relative paths, on the other in fact absolute paths.

That is what a gopackagesdriver actually gets. Minimal repro looks like the following.

package main

import (
	"encoding/json"
	"fmt"
	"os"

	"golang.org/x/tools/go/packages"
)

func main() {
	var req packages.DriverRequest

	err := json.NewDecoder(os.Stdin).Decode(&req)
	if err != nil {
		fmt.Fprintln(os.Stderr, err)
		os.Exit(1)
	}

	for path := range req.Overlay {
		fmt.Fprintln(os.Stderr, path)
		os.Exit(1)
	}
}

This driver is throwing an error that contains an absolute path.

What did you expect to see?

I expect the docs should not be misleading.

I think there is no reason for a driver to operate with relative paths.
So I would propose to fix the docs instead of changing the packages.Load(...) behavior.
@gopherbot gopherbot added the Tools This label describes issues relating to any tools in the x/tools repository. label May 21, 2024
@gopherbot gopherbot added this to the Unreleased milestone May 21, 2024
@cagedmantis cagedmantis added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label May 21, 2024
@cagedmantis
Copy link
Contributor

cc @matloob

@matloob
Copy link
Contributor

matloob commented May 21, 2024

I might be misunderstanding the bug report but I'm not sure I see the inconsistency.

The intention of the driverRequest doc is not to say that the path has to be absolute, but that relative paths are interpreted relative to the driver's working directory. Maybe we can be more clear and explicitly state that the path can be absolute?

@fxrlv
Copy link
Contributor Author

fxrlv commented May 21, 2024 •
edited
Loading

Oh, I see. Very first looking at the doc I thought that paths are supposed to be relative and that is the reason why that mark about driver’s workdir is there.

Having that paths can be both relative and absolute it’s unclear what’s expected behavior when one path is listed twice in both forms.
Maybe it’s also worth mentioning. What do you think?

@matloob
Copy link
Contributor

matloob commented May 21, 2024

I wouldn't mind if we added a clarifying comment. Using the default cmd/go driver it's an error to provide two paths that canonicalize to the same path in an overlay (https://cs.opensource.google/go/go/+/647870becc230b022b431a4ef8b7c9b31382db6c:src/cmd/go/internal/fsys/fsys.go;l=204)

Feel free to send a CL

@gabyhelp gabyhelp mentioned this issue Dec 31, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
4 participants
@cagedmantis @gopherbot @fxrlv @matloob