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: improve handling of symlinked module root for go mod (tidy) #45609

Open
thaJeztah opened this issue Apr 17, 2021 · 6 comments
Open

cmd/go: improve handling of symlinked module root for go mod (tidy) #45609

thaJeztah opened this issue Apr 17, 2021 · 6 comments

Comments

@thaJeztah
Copy link
Contributor

@thaJeztah thaJeztah commented Apr 17, 2021

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

$ go version
go version go1.16.3 darwin/amd64

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
(not relevant)

What did you do?

For easy navigation between projects, I have a "jump" directory on my machine,
in which I keep symlinks to frequently used projects. go mod tidy ignores
symlinks (and warns about ignoring them), which is generally ok, but in situations
where the whole module is detected to be in a "symlinked" path, it can be more
disruptive.

To reproduce this situation (you can run this inside a container to not clutter
up your host);

  1. have a Go project on your machine

    git clone https://github.com/moby/spdystream /go/src/github.com/moby/spdystream
  2. Create a "projects" directory with a symlink to the project

    mkdir -p /projects \
      && ln -s /go/src/github.com/moby/spdystream/ /projects/spdystream
  3. Navigate to the project using the symlink

    cd /projects/spdystream
  4. Notice that pwd shows the path of the symlink

    pwd
    /projects/spdystream

    (This is expected, and could be avoided by using cd -P instead of cd)

  5. Run go mod tidy (usually, this would be after updating go.mod to update some dependencies)

    go mod tidy
    warning: ignoring symlink /projects/spdystream
    go: warning: "all" matched no packages
  6. Find out that all dependencies were removed from go.mod, and go.sum tobe emptied

    git diff
    diff --git a/go.mod b/go.mod
    index d9b9ad5..ce733f8 100644
    --- a/go.mod
    +++ b/go.mod
    @@ -1,5 +1,3 @@
     module github.com/moby/spdystream
    
     go 1.13
    -
    -require github.com/gorilla/websocket v1.4.2
    diff --git a/go.sum b/go.sum
    index 85efffd..e69de29 100644
    --- a/go.sum
    +++ b/go.sum
    @@ -1,2 +0,0 @@
    -github.com/gorilla/websocket v1.4.2 h1:+/TMaTYc4QFitKJxsQ7Yye35DkWvkdLcvGKqM+x0Ufc=
    -github.com/gorilla/websocket v1.4.2/go.mod h1:YR8l580nyteQvAITg2hZ9XVh4b55+EU/adAjf1fMHhE=

While it's generally ok to ignore symlinks within the project/module (there must be reasons for that), it's somewhat disruptive in this situation; especially in situations where you just edited go.mod, and then see your changes being wiped.

What did you expect to see?

What I expected to see for step 5., is go mod (tidy) to either:

  • follow the symlink to /go/src/github.com/moby/spdystream/ and resolve modules from inside that context
  • or go mod tidy to produce an error instead of a warning, and terminate before modifying go.mod and go.sum

What did you see instead?

All dependencies removed from go.mod, and go.sum emptied

@toothrot toothrot added this to the Backlog milestone Apr 20, 2021
@toothrot
Copy link
Contributor

@toothrot toothrot commented Apr 20, 2021

/cc @bcmills @jayconrod @matloob

It might be worth adding a friendlier error message to this scenario, and not emptying out the go.mod and go.sum files.

@thaJeztah
Copy link
Contributor Author

@thaJeztah thaJeztah commented Apr 20, 2021

Either option would be fine with me; out of curiosity (I tried to find the back story on "ignoring the symlinks"); what problems could occur if (in this specific scenario) the symlink would not be ignored?

Also happy to open a pull request if there's consensus on the approach.

@bcmills
Copy link
Member

@bcmills bcmills commented Apr 21, 2021

This is probably a fencepost error somewhere in the go command. We certainly shouldn't ignore the module root if the entire module is a symlink.

FWIW, one reason we ignore symlinks in general is because if you commit a change in a repository containing a symlink, that symlink can't be reliably reproduced in the module cache. (Not all filesystems support symlinks, and not all symlinks are repo-relative.)

@thaJeztah
Copy link
Contributor Author

@thaJeztah thaJeztah commented Apr 21, 2021

FWIW, one reason we ignore symlinks in general is because if you commit a change in a repository containing a symlink, that symlink can't be reliably reproduced in the module cache. (Not all filesystems support symlinks, and not all symlinks are repo-relative.)

Ah, yes, that makes sense (symlinks can be a bit of a PITA)

So from the above;

  • "easy" / "quick fix" is to: "if isSymlink(path) && isModuleRoot(path)" then error and abort (before modifying)
  • "nicer" fix (but needs to be checked for possible side-effects; "if isSymlink(path) && isModuleRoot(path)" then "proceed as usual
@bcmills
Copy link
Member

@bcmills bcmills commented Apr 21, 2021

Exactly. I think we should go for the “nicer” fix — unlike for nested packages, I don't see a compelling reason to disallow symlinks at the module root.

@thaJeztah
Copy link
Contributor Author

@thaJeztah thaJeztah commented Apr 21, 2021

Yup; nicer fixes are always good.

Having a quick look at the link I included in my report; warning is coming from here;

if !fi.IsDir() {
if fi.Mode()&fs.ModeSymlink != 0 && want {
if target, err := fsys.Stat(path); err == nil && target.IsDir() {
fmt.Fprintf(os.Stderr, "warning: ignoring symlink %s\n", path)
}
}
return nil
}

I see a ModRoot() function used;

walkPkgs(ModRoot(), targetPrefix, pruneGoMod|pruneVendor)

Which (20 second glance) looks to be the path to the module root;

// ModRoot returns the root of the main module.
// It calls base.Fatalf if there is no main module.
func ModRoot() string {

Looks like it's using a variable to keep that, so repeatedly using it inside the walkFn should (at a glance) likely be ok.

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
3 participants