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.

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

cmd/go: follow root dir symlink in fsys.Walk (though not other symlinks in the tree) #50807

Closed
matloob opened this issue Jan 25, 2022 · 16 comments
Closed

cmd/go: follow root dir symlink in fsys.Walk (though not other symlinks in the tree) #50807

matloob opened this issue Jan 25, 2022 · 16 comments
Assignees
@bcmills @matloob
Labels
early-in-cycle A change that should be done early in the 3 month dev cycle. GoCommand cmd/go NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.21

Comments

@matloob
Copy link
Contributor

matloob commented Jan 25, 2022

cmd/go currently doesn't follow symlinks when looking for packages when module roots (or the current directory in the case of ...). Though we don't want to follow symlinks within modules, it is often confusing to users when their current working directory is a symlink. When walking a directory tree, use stat rather than lstat when reading the root directory to follow the symlink. This will allow users to use the go command as they'd expect when the working directory is a symlink following symlinks within modules.

@bcmills
@matloob matloob added this to the Go1.19 milestone Jan 25, 2022
@matloob matloob self-assigned this Jan 25, 2022
@bcmills
Copy link
Contributor

bcmills commented Jan 25, 2022

I filed #50804 for the equivalent behavior in filepath.Walk, although I'm not particularly hopeful that anything will change on that side.

@bcmills bcmills added GoCommand cmd/go NeedsFix The path to resolution is known, but the work has not been done. early-in-cycle A change that should be done early in the 3 month dev cycle. labels Jan 26, 2022
@gopherbot
Copy link

This issue is currently labeled as early-in-cycle for Go 1.19.
That time is now, so a friendly reminder to look at it again.

@sternopanko
Copy link

When is this going to be fixed? filepath.Walk does not allow for traversing links
Even providing an alternative interface like filepath.WalkWithDepth or filepath.WalkPotentiallyCatastrophicRecursion would be better than having to rewrite this

@ianlancetaylor
Copy link
Contributor

@sternopanko Go is an open source project, so if this is a problem then perhaps you could send a patch to fix it yourself.

Note that this issue is only about a symlink at the root of the tree, so we don't need a new filepath function to handle it, just an extra step before the call to Walk in the cmd/go code.

@bcmills
Copy link
Contributor

bcmills commented Jun 2, 2022

@sternopanko, this issue is about a specific workaround in cmd/go.

I filed #50804 for a more general filepath behavior, but withdrew that issue as it didn't seem worth a breaking change to me. (But note that on that issue I suggest that we treat the working directory as the symlink, not that we walk through the symlink.)

It is certainly possible to implement a Walk-like function that traverses symlinks outside of the standard library; see, for example, github.com/facebookgo/symwalk.

@sternopanko
Copy link

That facebook solution is a nice one
I already finished a solution locally, it is very simple to implement
Also, if we are in linux we can check the inode to see if we have been at this point before

@sternopanko
Copy link

@ianlancetaylor the solution is slightly less intuitive than you might think but is very easy to implement
The facebook solution should probably be part of go

@sternopanko
Copy link

Of course to detect a loop in the tree (at least for linux)
Just use the inode

@ianlancetaylor
Copy link
Contributor

@bcmills @matloob This issue is marked for 1.19. Should it move to Backlog? Thanks.

@bcmills bcmills modified the milestones: Go1.19, Backlog Jun 27, 2022
@matloob matloob changed the title cmd/go: use stat rather than lstat to read root dir in fsys.Walk cmd/go: follow root dir symlink in fsys.Walk (though not other symlinks in the tree) Nov 7, 2022
@gopherbot
Copy link

Change https://go.dev/cl/448360 mentions this issue: internal/fsys: follow root symlink in fsys.Walk

@gopherbot
Copy link

Change https://go.dev/cl/461959 mentions this issue: Revert "internal/fsys: follow root symlink in fsys.Walk"

@bcmills bcmills self-assigned this Jan 13, 2023
@bcmills bcmills modified the milestones: Backlog, Go1.21 Jan 13, 2023
@bcmills bcmills reopened this Jan 13, 2023
@bcmills
Copy link
Contributor

bcmills commented Jan 13, 2023 •
edited

Reopened because I reverted the attempted fix as https://go.dev/cl/461959.
(It introduced the regression reported in #57754, and upon inspection there were also likely some bad interactions with overlays.)

gopherbot pushed a commit that referenced this issue Jan 13, 2023
@gopherbot
Revert "internal/fsys: follow root symlink in fsys.Walk"
ebb572d 
This reverts CL 448360 and adds a regression test for #57754.

Reason for revert: broke 'go list' in Debian's distribution of the Go toolchain

Fixes #57754.
Updates #50807.

Change-Id: I3e8b9126294c55f21572774b549fb28f29eded0f
Reviewed-on: https://go-review.googlesource.com/c/go/+/461959
Reviewed-by: Russ Cox <rsc@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Auto-Submit: Bryan Mills <bcmills@google.com>
Run-TryBot: Bryan Mills <bcmills@google.com>
@gopherbot
Copy link

Change https://go.dev/cl/463179 mentions this issue: cmd/go: traverse module-root symlinks in Walk calls

@gopherbot
Copy link

Change https://go.dev/cl/463177 mentions this issue: os: make Lstat for symlinks on Windows consistent with POSIX

@gopherbot
Copy link

Change https://go.dev/cl/463176 mentions this issue: cmd/go/internal/str: fix PathPrefix functions for root directories

gopherbot pushed a commit that referenced this issue Jan 25, 2023
@gopherbot
os: make Lstat for symlinks on Windows consistent with POSIX
1d3088e 
This also makes path/filepath.Walk more consistent between
Windows and POSIX platforms.

According to
https://pubs.opengroup.org/onlinepubs/9699919799.2013edition/basedefs/V1_chap04.html#tag_04_12
symlinks in a path that includes a trailing slash must be resolved
before a function acts on that path.

POSIX defines an lstat function, whereas the Win32 API does not, so
Go's os.Lstat should follow the (defined) POSIX semantics instead of
doing something arbitrarily different.

CL 134195 added a test for the correct POSIX behavior when os.Lstat is
called on a symlink. However, the test turned out to be broken on Windows,
and when it was fixed (in CL 143578) it was fixed with different Lstat
behavior on Windows than on all other platforms that support symlinks.

In #50807 we are attempting to provide consistent symlink behavior for
cmd/go. This unnecessary platform difference, if left uncorrected,
will make that fix much more difficult.

CL 460595 reworked the implementation of Stat and Lstat on Windows,
and with the new implementation this fix is straightforward.

For #50807.
Updates #27225.

Change-Id: Ia28821aa4aab6cefa021da2d9b803506cdb2621b
Reviewed-on: https://go-review.googlesource.com/c/go/+/463177
Reviewed-by: Quim Muntal <quimmuntal@gmail.com>
Auto-Submit: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Alex Brainman <alex.brainman@gmail.com>
Reviewed-by: Russ Cox <rsc@golang.org>
Run-TryBot: Bryan Mills <bcmills@google.com>
gopherbot pushed a commit that referenced this issue Jan 25, 2023
@gopherbot
cmd/go/internal/str: fix PathPrefix functions for root directories
77cd3a4 
For #51506.
For #50807.

Change-Id: I4c0ae85a2103ac4f07351a4f01ce24fa02f03104
Reviewed-on: https://go-review.googlesource.com/c/go/+/463176
Run-TryBot: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Auto-Submit: Bryan Mills <bcmills@google.com>
Reviewed-by: Russ Cox <rsc@golang.org>
johanbrandhorst pushed a commit to Pryz/go that referenced this issue Feb 12, 2023
@johanbrandhorst
cmd/go: traverse module-root symlinks in Walk calls
e311275 
fsys.Walk, like filepath.Walk, avoids traversing symlinks. Also like
filepath.Walk, it follows a symlink at the root if the root path ends
in a file separator (consistent with POSIX pathname resolution¹).

If the user's working directory is within a repository stored in
(and symlinked to) a different filesystem path, we want to follow the
symlink instead of treating the module as completely empty.

¹https://pubs.opengroup.org/onlinepubs/9699919799.2013edition/basedefs/V1_chap04.html#tag_04_12

Fixes golang#50807.
Updates golang#57754.

Change-Id: Idaf6168dfffafe879e05b4ded5fda287fcd3eeec
Reviewed-on: https://go-review.googlesource.com/c/go/+/463179
Auto-Submit: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Russ Cox <rsc@golang.org>
Run-TryBot: Bryan Mills <bcmills@google.com>
@gopherbot
Copy link

Change https://go.dev/cl/509098 mentions this issue: doc/go1.21: add a release note for CL 463177

gopherbot pushed a commit that referenced this issue Jul 12, 2023
@gopherbot
doc/go1.21: add a release note for CL 463177
230e549 
I'm not sure why the relnote tool did not fill in a TODO for that
change; one was requested in
http://go.dev/cl/c/go/+/463177/3#message-87065dffb06e196fba9a325fefb32f16b41b6b15.

Updates #50807.
Updates #27225.

Change-Id: If6be8c126bcafb04ba92de88d2fc74a0557a07b8
Reviewed-on: https://go-review.googlesource.com/c/go/+/509098
Reviewed-by: Cherry Mui <cherryyz@google.com>
Auto-Submit: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Bryan Mills <bcmills@google.com>
bradfitz pushed a commit to tailscale/go that referenced this issue Jul 15, 2023
@bradfitz
doc/go1.21: add a release note for CL 463177
83ffbf3 
I'm not sure why the relnote tool did not fill in a TODO for that
change; one was requested in
http://go.dev/cl/c/go/+/463177/3#message-87065dffb06e196fba9a325fefb32f16b41b6b15.

Updates golang#50807.
Updates golang#27225.

Change-Id: If6be8c126bcafb04ba92de88d2fc74a0557a07b8
Reviewed-on: https://go-review.googlesource.com/c/go/+/509098
Reviewed-by: Cherry Mui <cherryyz@google.com>
Auto-Submit: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Bryan Mills <bcmills@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@bcmills bcmills

@matloob matloob

Labels
early-in-cycle A change that should be done early in the 3 month dev cycle. GoCommand cmd/go NeedsFix The path to resolution is known, but the work has not been done.
Projects
Release Status & Release Blockers
Status: Done
Milestone
Go1.21
Development

No branches or pull requests
5 participants
@ianlancetaylor @bcmills @gopherbot @matloob @sternopanko