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

go/build: document that Context.HasSubdir does not check existence of dir #17888

Closed
dmitshur opened this issue Nov 11, 2016 · 2 comments
Closed
Labels
Documentation FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@dmitshur
Copy link
Contributor

dmitshur commented Nov 11, 2016

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

$ go version
go version go1.7.3 darwin/amd64

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

$ go env
GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/Dmitri/Dropbox/Work/2013/GoLanding:/Users/Dmitri/Dropbox/Work/2013/GoLand"
GORACE=""
GOROOT="/usr/local/go"
GOTOOLDIR="/usr/local/go/pkg/tool/darwin_amd64"
CC="clang"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/tw/kgz4v2kn4n7d7ryg5k_z3dk40000gn/T/go-build353413060=/tmp/go-build -gno-record-gcc-switches -fno-common"
CXX="clang++"
CGO_ENABLED="1"

What did you do?

I read the documentation for Context.HasSubdir:

https://godoc.org/go/build#Context.HasSubdir

// HasSubdir reports whether dir is a subdirectory of
// (perhaps multiple levels below) root.
// If so, HasSubdir sets rel to a slash-separated path that
// can be joined to root to produce a path equivalent to dir.
// If HasSubdir is nil, Import uses an implementation built on
// filepath.EvalSymlinks.
HasSubdir func(root, dir string) (rel string, ok bool)

What did you expect to see?

Based on the name of the func and the phrase "whether dir is a subdirectory of root", I expected the default implementation (when HasSubdir is nil), which uses filepath.EvalSymlinks, to return false if the directory doesn't exist.

I also asked another person, and that was their understanding of the expected behavior too.

What did you see instead?

After looking into the implementation, I learned that the default implementation (and therefore, all custom implementation that are expected to act in the same way) does not try to ensure that dir is a subdirectory of root.

Instead, it simply reports "whether dir is a path that points to what would be a subdirectory (possibly multiple levels below) of root".

In other words, it does not check that dir is a directory that actually exists. It simply does a lexical path check (also uses filepath.EvalSymlinks to resolve potential symlinks).


This is one of those situations where I'm not completely sure if this is a bug in the implementation, or a case of slightly poor/misleading documentation. Looking more into the history of the relevant code, and the fact that IsDir exists solely to check existence of a directory, I am 80%+ confident the current behavior is correct and this is just a documentation issue.

Update: After spending more time with the code, I am now 97%+ confident the current behavior is correct, and this is just a documentation/naming issue. IsDir exists for checking if a directory exists, so it's not the responsibility of HasSubdir to check that too.

What do other people think about the current phrasing?

I think perhaps it should be clarified that dir does not need to exist for HasSubdir to return true.

Maybe it can be clarified by rewriting it to be something like this (except better)?

// HasSubdir reports whether dir is a path that points to
// what would be a subdirectory of (perhaps multiple levels below) root
// (but it does not try to check that dir currently exists).
// If so, HasSubdir sets rel to a slash-separated path that
// can be joined to root to produce a path equivalent to dir.
// If HasSubdir is nil, Import uses an implementation built on
// filepath.EvalSymlinks.
HasSubdir func(root, dir string) (rel string, ok bool)
@gopherbot
Copy link

CL https://golang.org/cl/33158 mentions this issue.

@quentinmit quentinmit added Documentation NeedsFix The path to resolution is known, but the work has not been done. labels Nov 14, 2016
@quentinmit quentinmit added this to the Go1.8Maybe milestone Nov 14, 2016
@quentinmit quentinmit changed the title go/build: Not very clear that Context.HasSubdir does not check existence of dir. go/build: document that Context.HasSubdir does not check existence of dir Nov 14, 2016
@gopherbot
Copy link

CL https://golang.org/cl/34240 mentions this issue.

gopherbot pushed a commit that referenced this issue Mar 4, 2017
When calling build.Import, normally, an error is returned if the
directory doesn't exist. However, that didn't happen for local
import paths when build.FindOnly ImportMode was used.

This change fixes that, and adds tests. It also makes the error
value more consistent in all scenarios where it occurs.

When calling build.Import with a local import path, the package
can only exist in a single deterministic directory. That makes
it possible verify that directory exists earlier in the path,
and return a "cannot find package" error if it doesn't.
Previously, this occurred only when build.FindOnly ImportMode
was not set. It occurred quite late, after getting past Found
label, to line that calls ctxt.readDir. Doing so would return
an error like "no such file or directory" when the directory
does not exist.

Fixes #17863.
Updates #17888 (relevant issue I ran into while working on this CL).

Change-Id: If6a6996ac6176ac203a88bd31419748f88d89d7c
Reviewed-on: https://go-review.googlesource.com/33158
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@golang golang locked and limited conversation to collaborators Dec 9, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Documentation FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

3 participants