godep does not respect +build tags, latest golang.org/x/net/context breaks #448

Closed
themartorana opened this Issue Apr 5, 2016 · 10 comments

Comments

Projects
None yet
4 participants
@themartorana

The latest commit to the context package contains the file go17.go which imports the context library from the Go 1.7 standard library.

Despite having the // +build go1.7 tag at the top of the file, godep parses the file and throws a godep: Package (context) not found error.

I can get past this by going into the context package and checking out the release-branch.go1.6 branch, but this simply illustrates the fact that godep isn't checking the +build tags, and is attempting to import packages for non-installed versions of Go.

@themartorana themartorana changed the title from Latest golang.org/x/net/context package breaks godep to godep does not respect +build tags, latest golang.org/x/net/context breaks Apr 5, 2016

@themartorana themartorana reopened this Apr 5, 2016

@themartorana

This comment has been minimized.

Show comment
Hide comment
@themartorana

themartorana Apr 5, 2016

Sorry, tapped the wrong button...

Sorry, tapped the wrong button...

@CannibalVox

This comment has been minimized.

Show comment
Hide comment
@CannibalVox

CannibalVox Apr 5, 2016

It's worse than that- deleting go17.go wont' help because it actually refused to read pre_go17.go. It's like build tag is reversed, it will ONLY read the 1.7 file.

It's worse than that- deleting go17.go wont' help because it actually refused to read pre_go17.go. It's like build tag is reversed, it will ONLY read the 1.7 file.

@freeformz

This comment has been minimized.

Show comment
Hide comment
@freeformz

freeformz Apr 5, 2016

Contributor

What version of godep? Versions of godep (circa v41+, I would need to check the changelog for the specific version) process all .go files, except for those with "ignore" and "appengine" build tags. Adding go version build tags shouldn't be that hard since we also store the major go version in the Godeps.json file though.

Sent from my iPhone

On Apr 5, 2016, at 10:17, Cannibal Vox notifications@github.com wrote:

It's worse than that- deleting go17.go wont' help because it actually refused to read pre_go17.go. It's like build tag is reversed, it will ONLY read the 1.7 file.


You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub

Contributor

freeformz commented Apr 5, 2016

What version of godep? Versions of godep (circa v41+, I would need to check the changelog for the specific version) process all .go files, except for those with "ignore" and "appengine" build tags. Adding go version build tags shouldn't be that hard since we also store the major go version in the Godeps.json file though.

Sent from my iPhone

On Apr 5, 2016, at 10:17, Cannibal Vox notifications@github.com wrote:

It's worse than that- deleting go17.go wont' help because it actually refused to read pre_go17.go. It's like build tag is reversed, it will ONLY read the 1.7 file.


You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub

@themartorana

This comment has been minimized.

Show comment
Hide comment
@themartorana

themartorana Apr 5, 2016

For me: godep v60 (darwin/amd64/go1.6)

For me: godep v60 (darwin/amd64/go1.6)

@CannibalVox

This comment has been minimized.

Show comment
Hide comment
@CannibalVox

CannibalVox Apr 5, 2016

Above PR fixes the issue, hopefully we can see the resolution live soon, this is a pretty serious deal :\

Above PR fixes the issue, hopefully we can see the resolution live soon, this is a pretty serious deal :\

@freeformz

This comment has been minimized.

Show comment
Hide comment
@freeformz

freeformz Apr 6, 2016

Contributor

Looks like go1.7 includes a package named "context" and if you are not on 1.7 (really devel) then godep can't find the package because you don't actually have it.

Contributor

freeformz commented Apr 6, 2016

Looks like go1.7 includes a package named "context" and if you are not on 1.7 (really devel) then godep can't find the package because you don't actually have it.

freeformz added a commit that referenced this issue Apr 6, 2016

Handle go version tags
Fixes #448

Obey the go tool's definition of how the go versions build tags work for
the purposes of dependency scanning.

Don't use the current go version, but the recorded go version in
Godeps.json.

FIXME: We still copy the files. This is sub optimal, but probably fine
in the 80% case where people are using the version of go they have
specified in the Godeps.json file. Longer term we need to obey the
gofiles / ignore lists from the package list.
@freeformz

This comment has been minimized.

Show comment
Hide comment
@freeformz

freeformz Apr 6, 2016

Contributor

Can anyone test out the branch that I pushed for this (See #451) ? I tested it here, but it's late and I don't want to merge it before reviewing it in the morning (and/or getting other +1s).

Contributor

freeformz commented Apr 6, 2016

Can anyone test out the branch that I pushed for this (See #451) ? I tested it here, but it's late and I don't want to merge it before reviewing it in the morning (and/or getting other +1s).

@ddgenome

This comment has been minimized.

Show comment
Hide comment
@ddgenome

ddgenome Apr 6, 2016

@freeformz I can confirm that using PR #451 godep save -v on a project that imports "golang.org/x/net/context" finishes successfully whereas it fails with godep: Package (context) not found when using master.

ddgenome commented Apr 6, 2016

@freeformz I can confirm that using PR #451 godep save -v on a project that imports "golang.org/x/net/context" finishes successfully whereas it fails with godep: Package (context) not found when using master.

@themartorana

This comment has been minimized.

Show comment
Hide comment

@freeformz seconded.

@freeformz freeformz closed this in 35ee059 Apr 6, 2016

@freeformz

This comment has been minimized.

Show comment
Hide comment
@freeformz

freeformz Apr 6, 2016

Contributor

Thanks everyone, this should now be fixed.

Contributor

freeformz commented Apr 6, 2016

Thanks everyone, this should now be fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment