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: PWD not honored in "go list -m" with explicit GOWORK #51823

Closed
leitzler opened this issue Mar 20, 2022 · 11 comments
Closed

cmd/go: PWD not honored in "go list -m" with explicit GOWORK #51823

leitzler opened this issue Mar 20, 2022 · 11 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@leitzler
Copy link
Contributor

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

$ go version
go version devel go1.19-4d2da99 Sun Mar 20 03:01:15 2022 +0000 darwin/arm64
$ go1.18 version
go version go1.18 darwin/arm64

Does this issue reproduce with the latest release?

Yes

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

go env Output
$ go env
N/A

What did you do?

A simple reproducable is to create a workspace within the temporary directory on darwin:

cd $(mktemp -d) && txtar-x <<<'
-- go.work --
go 1.19

use ./mymod
-- mymod/go.mod --
module mymod

go 1.19
-- mymod/main.go --
package main
'

The temp dir on macOS is a symlink where the real path has a /private prefix, e.g.:

$ pwd
/var/folders/cz/31br911566l_bbx72cs_pgs40000gn/T/tmp.gRKXUCNX
$ pwd -P
/private/var/folders/cz/31br911566l_bbx72cs_pgs40000gn/T/tmp.gRKXUCNX

x/tools relies on cmd/go returning resolved paths as per: https://github.com/golang/tools/blob/5ea13d0d89f92d5ca5468e282dd4ba2ad7503564/internal/gocommand/invoke.go#L216-L221

	// On darwin the cwd gets resolved to the real path, which breaks anything that
	// expects the working directory to keep the original path, including the
	// go command when dealing with modules.
	// The Go stdlib has a special feature where if the cwd and the PWD are the
	// same node then it trusts the PWD, so by setting it in the env for the child
	// process we fix up all the paths returned by the go command.

When running go list, the directories works as expected:

$ go list -json mymod/... | grep $(pwd)
	"Dir": "/var/folders/cz/31br911566l_bbx72cs_pgs40000gn/T/tmp.gRKXUCNX/mymod",
	"Root": "/var/folders/cz/31br911566l_bbx72cs_pgs40000gn/T/tmp.gRKXUCNX/mymod",
		"Dir": "/var/folders/cz/31br911566l_bbx72cs_pgs40000gn/T/tmp.gRKXUCNX/mymod",
		"GoMod": "/var/folders/cz/31br911566l_bbx72cs_pgs40000gn/T/tmp.gRKXUCNX/mymod/go.mod",
$ PWD=$(pwd -P) go list -json mymod/... | grep $(pwd)
	"Dir": "/private/var/folders/cz/31br911566l_bbx72cs_pgs40000gn/T/tmp.gRKXUCNX/mymod",
	"Root": "/private/var/folders/cz/31br911566l_bbx72cs_pgs40000gn/T/tmp.gRKXUCNX/mymod",
		"Dir": "/private/var/folders/cz/31br911566l_bbx72cs_pgs40000gn/T/tmp.gRKXUCNX/mymod",
		"GoMod": "/private/var/folders/cz/31br911566l_bbx72cs_pgs40000gn/T/tmp.gRKXUCNX/mymod/go.mod",

Until I explicitly set GOWORK (before PWD since go env otherwise picks up the resolved dir):

$ GOWORK=$(go env GOWORK) PWD=$(pwd -P) go list -json mymod/... | grep $(pwd)
	"Dir": "/var/folders/cz/31br911566l_bbx72cs_pgs40000gn/T/tmp.gRKXUCNX/mymod",
	"Root": "/var/folders/cz/31br911566l_bbx72cs_pgs40000gn/T/tmp.gRKXUCNX/mymod",
		"Dir": "/var/folders/cz/31br911566l_bbx72cs_pgs40000gn/T/tmp.gRKXUCNX/mymod",
		"GoMod": "/var/folders/cz/31br911566l_bbx72cs_pgs40000gn/T/tmp.gRKXUCNX/mymod/go.mod",

What did you expect to see?

I would expect that PWD is honored when running go list regardless of if GOWORK is set or not. Essentially I would expect GOWORK=$(go env GOWORK) here to be a no-op without side effects.

What did you see instead?

go list returning non-resolved paths when PWD is set.

@leitzler
Copy link
Contributor Author

//cc @bcmills @matloob

@gopherbot
Copy link

Change https://go.dev/cl/394054 mentions this issue: internal/lsp/lsprpc: resolve GOWORK real path when getting go envs

@bcmills
Copy link
Member

bcmills commented Mar 20, 2022

I would expect GOWORK=$(go env GOWORK) here to be a no-op without side effects.

It should be, provided that you run go env GOWORK with the same value of PWD. If you set GOWORK and PWD to use two different names for the same directory, then one of them has to win out.

This doesn't look like a cmd/go bug to me.

@leitzler
Copy link
Contributor Author

Apologies, I should have explained that better. Another example (in linux, for readability):

$ cd /tmp && mkdir -p real/mymod
$ ln -s real symlink
$ cd symlink/mymod && go mod init mymod
$ pushd .. && go work init mymod/ && popd

Now from /tmp/symlink:

Plain go list uses /tmp/symlink (as I expected)

$ go list -json mymod/... | jq .Module
{
  "Path": "mymod",
  "Main": true,
  "Dir": "/tmp/symlink/mymod",
  "GoMod": "/tmp/symlink/mymod/go.mod",
  "GoVersion": "1.19"
}

Setting PWD to the real path makes go list use /tmp/real (also expected):

$ PWD=/tmp/real/mymod go list -json mymod/... | jq .Module
{
  "Path": "mymod",
  "Main": true,
  "Dir": "/tmp/real/mymod",
  "GoMod": "/tmp/real/mymod/go.mod",
  "GoVersion": "1.19"
}

Setting both PWD to the real path and an explicit GOMOD (to the symlink) also makes go list use PWD (expected):

$ PWD=/tmp/real/mymod GOMOD=/tmp/symlink/mymod/go.mod go list -json mymod/... | jq .Module
{
  "Path": "mymod",
  "Main": true,
  "Dir": "/tmp/real/mymod",
  "GoMod": "/tmp/real/mymod/go.mod",
  "GoVersion": "1.19"
}

But setting GOWORK changes the behavior and PWD is ignored. When you say that:

one of them has to win out.

I would expect PWD to win, as it does when I set GOMOD explicitly in the example above.

$ PWD=/tmp/real/mymod GOWORK=/tmp/symlink/go.work go list -json mymod/... | jq .Module
{
  "Path": "mymod",
  "Main": true,
  "Dir": "/tmp/symlink/mymod",
  "GoMod": "/tmp/symlink/mymod/go.mod",
  "GoVersion": "1.19"
}

@bcmills
Copy link
Member

bcmills commented Mar 20, 2022

Setting GOMOD has no effect at all; see #51217.

@leitzler
Copy link
Contributor Author

Gotcha, I incorrectly assumed that you could set GOMOD. Regardless of that I still expect PWD to have the same effect on module/package file paths even when GOWORK is explicitly set.

The current situation, if I understand it correctly, is that paths is shown based on the use-directive in "go.work-mode". And that path can be relative such as the example above use ./mymod.

The difference between setting GOWORK explicitly and letting cmd/go infer GOWORK is that in one case PWD will affect the result, while in the other case it won't.

@bcmills
Copy link
Member

bcmills commented Mar 20, 2022

The difference you're seeing is that when PWD is set, the inferred value of GOWORK reflects that setting. If you explicitly set GOWORK=$(PWD=… go env GOWORK), you should find that the result is consistent with setting PWD to that value for other commands.

@leitzler
Copy link
Contributor Author

Yeah, I understand that the inferred value of GOWORK depends on PWD.

The part that seems odd is that directories provided by go list -json depends on PWD unless GOWORK is set.

Ok, just to make sure I understand how this suppose to work:

  • Directories returned by go list -json is evaluated by looking at the use-directive in go.work (in go.work-mode obviously).
  • If the use-directive is a relative path then the absolute path will be evaluated by looking at the directory part of GOWORK.
  • Setting PWD will alter how GOWORK is inferred, unless is explicitly set.

Am I missing something?

@bcmills
Copy link
Member

bcmills commented Mar 20, 2022

Yes, that's exactly it. When you set PWD without GOWORK, it changes the inferred GOWORK, which in turn changes all of the paths computed relative to that.

When you set GOWORK explicitly, you override the inferred GOWORK, which changes all of the paths derived from it.

@mknyszek mknyszek added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Mar 21, 2022
@mknyszek mknyszek added this to the Go1.19 milestone Mar 21, 2022
@mknyszek
Copy link
Contributor

Adding this to the Go 1.19 milestone until we can understand what the desired behavior is and what we should do about it (I didn't fully follow the conversation, but I also only skimmed it). Feel free to move it.

gopherbot pushed a commit to golang/tools that referenced this issue Apr 1, 2022
When using gopls remote, existing go env vars are injected in the
initialize message. Setting GOWORK explicitly will impact cmd/go
invocations, for example "go list" during package load. The reason it
impacts is that gopls manually sets PWD to fix paths returned by cmd/go
if the working directory is inside a symlinked directory.

By only propagate GOWORK when it is explicitly set we will ensure that
the behavior is the same when running with or without remote.

See golang/go#51823.

Fixes golang/go#51825

Change-Id: I6280aa7d8208e5aee269f19356668c7713e9f0a4
Reviewed-on: https://go-review.googlesource.com/c/tools/+/394054
Trust: Pontus Leitzler <leitzler@gmail.com>
Run-TryBot: Pontus Leitzler <leitzler@gmail.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
@bcmills
Copy link
Member

bcmills commented Apr 19, 2022

I don't think there is anything to change here. The go command is correctly interpreting the current directory per PWD, and is correctly interpolating GOWORK relative to PWD.

If GOWORK is correctly set to a value that matches PWD, then everything lines up. If PWD is changed to no longer match GOWORK, then GOWORK must win, or else the command would fail entirely because the requested packages aren't lexically inside the workspace. (The latter seems strictly worse than the former.)

Overall, my advice is: keep PWD consistent with whatever the user set it to, and don't mess with it otherwise. (When changing to a relative directory, set PWD relative to the previous PWD.)

@bcmills bcmills closed this as completed Apr 19, 2022
@golang golang locked and limited conversation to collaborators Apr 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

4 participants