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: vendoring fail when put on $GOPATH root with Import Path Checking #12232

Closed
kurze opened this issue Aug 20, 2015 · 14 comments
Closed

cmd/go: vendoring fail when put on $GOPATH root with Import Path Checking #12232

kurze opened this issue Aug 20, 2015 · 14 comments
Labels
FrozenDueToAge
Milestone
Go1.5.1

Comments

@kurze
Copy link

kurze commented Aug 20, 2015

I have an issue with vendoring when placed on $GOPATH/src/vendor and "vendored" file use Import Path Checking.

$GOPATH
.
└── src
    ├── main
    │   └── main.go
    └── vendor
        └── vnd.org
            └── a
                └── a.go

with main.go :

package main

import _ "vnd.org/a"

func main() {}

and a.go :

package a // import "vnd.org/a"

go build throw many errors like this :

main.go:3:8: code in directory /[...]/src/vendor/vnd.org/a expects import "vnd.org/a"

According to the spec : "Code inside vendor/ subtrees is not subject to import path checking.".

However import path checking is not properly disabled if the vendored package is at the root of GOPATH since the package name doesn't contain "/vendor/" but starts with "vendor/"
The fix looks easy enough ( replace the condition in file cmd/go/pkg.go line 371 with (!go15VendorExperiment || (!strings.Contains(path, "/vendor/") && !strings.HasPrefix(path, "vendor/"))) ), I can submit a pull request if necessary.
@adg
Copy link
Contributor

adg commented Aug 20, 2015

Putting a vendor directory at the root of a workspace directory $GOPATH/src is somewhat nonsensical. There's barely a distinction between doing that and not creating the vendor directory at all.

We should probably disallow it.

@vanackere
Copy link
Contributor

This bug make it impossible to locally vendor many golang.org/x/... packages for example. It would be nice to have this fixed in 1.5.x

@adg
Copy link
Contributor

adg commented Aug 20, 2015

cc @davecheney

@vanackere
Copy link
Contributor

I don't see anything wrong with putting a vendor directory at the root of a workspace directory (eg if the intended usage is to make use of external tools to manage this part of the code).

In this particular case, the fix looks also especially obvious and in line with the rest of the code (see func findVendor, in cmd/go/pkg.go line 639:

    switch {
    case strings.Contains(path, "/vendor/"):
        return strings.LastIndex(path, "/vendor/") + 1, true
    case strings.HasPrefix(path, "vendor/"):
        return 0, true
    }

@gopherbot
Copy link

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

@mattfarina
Copy link

Can someone explain the use case for vendor/ being right inside $GOPTH/src? I'm attempting to understand when one would put it there.

@pkieltyka
Copy link

yea, this sounds strange to me too.. sounds like its useful when doing $GOPATH rewriting for project workspaces...?

I personally wish for vendor/ to have the subdirectories: src/ and pkg/

@mattfarina
Copy link

@pkieltyka why have those subdirectories inside vendor/? The vendor/ directory and its subdirectories live inside the other directories off the GOPATH, for example pkg.

@pkieltyka
Copy link

@mattfarina I'd like to see src/ and pkg/ in the vendor/ of my project so I could use vendor/ dependencies for development. In order to use them for development, I need the pkg/ folder so that the vendored deps won't be rebuilt each time. With vendor/pkg, I can keep doing incremental builds of my project and its deps which is essential for keeping development productive.

@mattfarina
Copy link

@pkieltyka Try going into your vendor/ directory and running go install ./.... I think that prebuilds them for you. I use Glide (the package manager) and my cohort wrote something similar to this with glide rebuild.

@pkieltyka
Copy link

I guess go install ./... in vendor/ would work, thanks

@ianlancetaylor ianlancetaylor added this to the Go1.5.1 milestone Aug 26, 2015
@ianlancetaylor
Copy link
Contributor

Marking this as 1.5.1 so that we can make a decision on whether to include it. I can't tell whether this is a real bug or not. If it's a real bug, it's new in 1.5, so fixing it in 1.5.1 is not entirely out of bounds.

@rsc rsc closed this as completed in b55c4a0 Aug 27, 2015
@pkieltyka
Copy link

lol.. all that discussion for a 1 line fix.. nice :) thanks guys

@gopherbot
Copy link

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

gopherbot pushed a commit that referenced this issue Sep 8, 2015
@vanackere @broady
[release-branch.go1.5] cmd/go: properly ignore import comments for ve…
1d11801 
…ndored packages rooted at GOPATH

Fixes #12232.

Change-Id: Ide3fb7f5fc5ae377ae8683fbb94fd0dc01480549
Reviewed-on: https://go-review.googlesource.com/13924
Reviewed-by: Russ Cox <rsc@golang.org>
Reviewed-on: https://go-review.googlesource.com/14228
Reviewed-by: Ian Lance Taylor <iant@golang.org>
@golang golang locked and limited conversation to collaborators Sep 4, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge
Projects
None yet
Milestone
Go1.5.1
Development

No branches or pull requests
7 participants
@pkieltyka @mattfarina @vanackere @kurze @ianlancetaylor @adg @gopherbot