Skip to content

go/build: build.Import with FindOnly doesn't check if dir exists for local import paths (it works fine on full import paths, and if FindOnly is not used) #17863

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
Closed
dmitshur opened this issue Nov 9, 2016 · 3 comments
Closed

go/build: build.Import with FindOnly doesn't check if dir exists for local import paths (it works fine on full import paths, and if FindOnly is not used) #17863

dmitshur opened this issue Nov 9, 2016 · 3 comments
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone
Go1.9Maybe

Comments

@dmitshur
Copy link
Contributor

dmitshur commented Nov 9, 2016 •
edited
Loading

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-build319067970=/tmp/go-build -gno-record-gcc-switches -fno-common"
CXX="clang++"
CGO_ENABLED="1"

What did you do?

When calling build.Import, if a matching directory doesn't exist, in most cases an error will be returned.

For normal import paths, such an error is returned both with zero ImportMode (in other words, without build.FindOnly flag), and with build.FindOnly flag.

For relative import paths, such an error is returned is returned when you call build.Import with zero ImportMode.

However, when calling build.Import with build.FindOnly flag on a relative import path, there is no such error returned, instead nil error is returned.

For example, when calling:

_, err := build.Import("./doesnotexist", wd, build.FindOnly)

The returned err will be nil, even if the directory for package ./doesnotexist does not exist.

Here's a full reproduce program:

https://play.golang.org/p/cW_R2iaF1A

What did you expect to see?

The first check, using local import path, should've detected that the directory doesn't exist and returned an error.

finding "./doesnotexist" import path:
can't find package "./doesnotexist" (first check, using relative import path)

That's what happens if a full import path is used, or build.FindOnly ImportMode is changed to default import mode.

What did you see instead?

First check doesn't return an error.

finding "./doesnotexist" import path:
can't find package "./doesnotexist" (second check, using absolute import path)

I believe it's a bug in build.Import for two reasons:

  1. Documentation for build.FindOnly says:

    // If FindOnly is set, Import stops after locating the directory
// that should contain the sources for a package. It does not
// read any files in the directory.

    Notice it says "Import stops after locating the directory that should contain the sources for a package". Not before locating the directory, which is what it does now.

  2. It's not consistent. When given a full import path with FindOnly, an error is returned. There's no reason the output of build.Import should differ based on which of multiple valid ways you've used to refer to the same package (i.e., via a full import path or local import path).
@dmitshur dmitshur mentioned this issue Nov 9, 2016
Closed
@dmitshur
Copy link
Contributor Author

dmitshur commented Nov 9, 2016 •
edited
Loading

I'm working on this and have a WIP fix.

First, I noticed that there are currently no tests that cover this, so I worked on adding a test. Does this test look reasonable (i.e., should it be expected to pass)?

// Want to get a not exist error when directory for package does not exist.
func TestImport_dirNotExist(t *testing.T) {
    testenv.MustHaveGoBuild(t) // really must just have source
    ctxt := Default
    ctxt.GOPATH = ""

    // isNotExist reports whether the error is known to
    // report that a package does not exist.
    isNotExist := func(err error) bool {
        return strings.HasPrefix(err.Error(), "cannot find package") ||
            strings.Contains(err.Error(), "no such file or directory")
    }

    tests := []struct {
        label        string
        path, srcDir string
        mode         ImportMode
    }{
        {"Import(full, 0)", "go/build/doesnotexist", "", 0},
        {"Import(local, 0)", "./doesnotexist", filepath.Join(ctxt.GOROOT, "src/go/build"), 0},
        {"Import(full, FindOnly)", "go/build/doesnotexist", "", FindOnly},
        {"Import(local, FindOnly)", "./doesnotexist", filepath.Join(ctxt.GOROOT, "src/go/build"), FindOnly},
    }
    for _, test := range tests {
        _, err := ctxt.Import(test.path, test.srcDir, test.mode)
        if err == nil || !isNotExist(err) {
            t.Errorf(`%s got error: %q, want package not exist error`, test.label, err)
        }
    }
}

That test currently fails on the Import(local, FindOnly) case, but succeeds in all others. I have a fix that makes it pass, and all other go/build tests pass.

@gopherbot
Copy link
Contributor

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

@quentinmit quentinmit added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Nov 14, 2016
@quentinmit quentinmit added this to the Go1.9Maybe milestone Nov 14, 2016
@quentinmit
Copy link
Contributor

This is too late for 1.8. Marking NeedsDecision because I'm not sure if the current behavior is actually intentional or not; I know we have lots of obscure rules around Import.

/cc @rsc

@vcabbage vcabbage mentioned this issue May 1, 2017
Closed
gopherbot pushed a commit that referenced this issue May 15, 2017
@dmitshur
go/build: return partial information on Import error, for local impor…
67e4712 
…t paths

Documentation of build.Import says:

	// If the path is a local import path naming a package that can be imported
	// using a standard import path, the returned package will set p.ImportPath
	// to that path.
	// ...
	// If an error occurs, Import returns a non-nil error and a non-nil
	// *Package containing partial information.

That behavior was previously untested, and broken by change in CL 33158.

Fix that by avoiding returning early on error for local import paths.
First, gather partial information, and only then check that the p.Dir
directory exists.

Add tests for this behavior.

Fixes #19769.
Fixes #20175 (duplicate of #19769).
Updates #17863.

Change-Id: I169cb35291099d05e02aaa3cb23a7403d1cc3657
Reviewed-on: https://go-review.googlesource.com/42350
Reviewed-by: Russ Cox <rsc@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@dmitshur dmitshur mentioned this issue Aug 25, 2017
Closed
@ianlancetaylor ianlancetaylor mentioned this issue Sep 18, 2017
Open
@golang golang locked and limited conversation to collaborators Mar 4, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
None yet
Milestone
Go1.9Maybe
Development

No branches or pull requests
3 participants
@quentinmit @dmitshur @gopherbot