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: honour GO15VENDOREXPERIMENT #12278

Closed
brknstrngz opened this issue Aug 23, 2015 · 33 comments

Comments

Projects
None yet
@brknstrngz
Copy link

commented Aug 23, 2015

Hi,

Any chance of goimports being taught about GO15VENDOREXPERIMENT? With go 1.5 it rewrites import paths (to $CWD/vendor/path/to/3rd/party/pkg) even when GO15VENDOREXPERIMENT is set.

Thanks
Vlad

@mikioh mikioh changed the title goimports does not honour GO15VENDOREXPERIMENT x/tools/cmd/goimports: does not honour GO15VENDOREXPERIMENT Aug 23, 2015

@mikioh mikioh added this to the Unreleased milestone Aug 23, 2015

@bradfitz

This comment has been minimized.

Copy link
Member

commented Aug 23, 2015

Yes, this needs to happen. Now that my projects are also using vendor directories, I've also been running into this.

/cc @adg @Sajmani @josharian

@ereyes01

This comment has been minimized.

Copy link

commented Sep 2, 2015

I also have issues with oracle and GO15VENDOREXPERIMENT and oracle, which I've initially reported here: fatih/vim-go#525

Is this correct place to raise an issue for oracle? If so should I open a new one, or report in this one?

Thanks, Eddy

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Sep 2, 2015

@ereyes01 Assuming you are talking about x/tools/cmd/oracle, then filing an issue on this issue tracker is the right thing to do. This particular issue is about goimports.

@minux

This comment has been minimized.

Copy link
Member

commented Sep 2, 2015

@kylewolfe

This comment has been minimized.

Copy link

commented Sep 2, 2015

@minux, this is what I've been trying to get my head around the last few days. Tools like goimports and gocode would need a significant refactor and very temporary workaround to implement support for the vendor experiment.

Is there a reason that the vendor experiment was implemented only in the go tool rather than in go/build?

@kylewolfe

This comment has been minimized.

Copy link

commented Sep 4, 2015

@rsc, Is it possible and even worth it at this point to move the vendor functionality from cmd/go to go/build to enable third party tools to piggy back on the existing vendor functionality?

@minux

This comment has been minimized.

Copy link
Member

commented Sep 4, 2015

On Sep 4, 2015 11:29 AM, "Kyle Wolfe" notifications@github.com wrote:

@rsc, Is it possible and even worth it at this point to move the vendor
functionality from cmd/go to go/build to enable third party tools to piggy
back on the existing vendor functionality?

One issue is that the new go/build package will only be available in Go
1.6. I don't know if that's acceptable.

@pwaller

This comment has been minimized.

Copy link
Contributor

commented Sep 22, 2015

Could this be put onto a near-term milestone? (1.6?) I'm happily using the new vendoring mechanism and not having these source manipulation tools at our disposal is unfortunate. I'm also happy to have a crack at doing something if there is a consensus about what is a reasonable approach to fix it. Waiting until 1.6 is annoying, but waiting until after 1.6 would be worse.

@bradfitz

This comment has been minimized.

Copy link
Member

commented Sep 22, 2015

Goimports is not distributed with Go so we don't use the Go 1.N milestones for it. This can be fixed whenever.

@pwaller

This comment has been minimized.

Copy link
Contributor

commented Sep 22, 2015

@bradfitz I was referring to @minux's suggestion that the fix might involve fixing go/build. There are actually lots of things in need of fixing, and many of them are using go/build, so it is a logical thing that might need attention.

@pwaller

This comment has been minimized.

Copy link
Contributor

commented Sep 22, 2015

For example, goimports uses go/build here: https://github.com/golang/tools/blob/5a90b2958f12ca0f046ffe2eb1523c5896af5dae/imports/fix.go#L158 - which implies either the stdlib needs fixing or the whole importing logic needs to move out of the stdlib.

@minux minux changed the title x/tools/cmd/goimports: does not honour GO15VENDOREXPERIMENT go/build: honour GO15VENDOREXPERIMENT Sep 22, 2015

@minux minux modified the milestones: Go1.6Early, Unreleased Sep 22, 2015

@minux

This comment has been minimized.

Copy link
Member

commented Sep 22, 2015

@pwaller

This comment has been minimized.

Copy link
Contributor

commented Oct 19, 2015

Ping.

I note that the 1.6Early deadline (according to the github milestone) is October the 31st, which is now ~12 days away. Is this issue on anybody's list, and is there anything I can do to help? Has the deadline already been missed, so to say? I would really like to be able to use gorename and friends in Go 1.6. It is already hurting.

@adg

This comment has been minimized.

Copy link
Contributor

commented Oct 20, 2015

The deadline hasn't been missed.
But we should get moving on this.
If someone could investigate what's involved in making this work, that
would be great.

On 19 October 2015 at 21:39, Peter Waller notifications@github.com wrote:

Ping.

I note that the 1.6Early deadline (according to the github milestone) is
October the 31st, which is now ~12 days away. Is this issue on anybody's
list, and is there anything I can do to help? Has the deadline already been
missed, so to say? I would really like to be able to use gorename and
friends in Go 1.6. It is already hurting.


Reply to this email directly or view it on GitHub
#12278 (comment).

@sp-adan-lobato

This comment has been minimized.

Copy link

commented Nov 4, 2015

I'd like also this issue to be fixed! 👍

@gonzaloserrano

This comment has been minimized.

Copy link

commented Nov 4, 2015

Me too! Waiting to be able to pass all these awesome linters with the vendors' code instead of the global one.

@bradfitz

This comment has been minimized.

Copy link
Member

commented Nov 4, 2015

Many people want this. No need to enumerate the list of them here.

@kylewolfe

This comment has been minimized.

Copy link

commented Nov 4, 2015

I sent an email to golang-dev to solicit more ideas about go/build.

@cespare, can you link to discussion, please?

@cespare

This comment has been minimized.

Copy link
Contributor

commented Nov 4, 2015

@kylewolfe

This comment has been minimized.

Copy link

commented Nov 4, 2015

Should we also discuss how internal is handled? Currently internal packages are resolved at cmd/go as well. Should this be handled in go/build? If not, are we ok with internal continuing to be resolved by cmd/go while vendor is handled by go/build?

@davecheney

This comment has been minimized.

Copy link
Contributor

commented Nov 4, 2015

@kylewolfe vendor is also implemented in cmd/go, not go/build

https://go-review.googlesource.com/#/c/10923/

@kylewolfe

This comment has been minimized.

Copy link

commented Nov 4, 2015

@davecheney, Yes. The discussion is around moving vendor from cmd/go to go/build.

@davecheney

This comment has been minimized.

Copy link
Contributor

commented Nov 4, 2015

@kylewolfe I'm sorry I misunderstood what you mean by "If not, are we ok with internal continuing to be resolved by cmd/go while vendor is handled by go/build?"

@cespare

This comment has been minimized.

Copy link
Contributor

commented Nov 4, 2015

Please, this issue is already far too muddled without talking about internal packages as well. If you wish to discuss internal packages, please use the mailing list (or open a new issue if you feel there's a bug).

@kylewolfe

This comment has been minimized.

Copy link

commented Nov 4, 2015

@cespare, I'm sorry, but I think we should discuss where this lives, since it's doing almost exactly the same thing.

@dsymonds

This comment has been minimized.

Copy link
Member

commented Nov 4, 2015

internal is easy to implement, and doesn't impact import paths. It doesn't need to be discussed on this issue, which is specifically about vendor.

@kylewolfe

This comment has been minimized.

Copy link

commented Nov 4, 2015

@davecheney, I could have phrased my entire post a little better :)

Here is the internal implementation review: https://codereview.appspot.com/120600043/

@gopherbot

This comment has been minimized.

Copy link

commented Dec 11, 2015

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

@gopherbot

This comment has been minimized.

Copy link

commented Dec 11, 2015

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

@gopherbot

This comment has been minimized.

Copy link

commented Dec 11, 2015

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

@rsc rsc closed this in 0c428a5 Dec 16, 2015

rsc added a commit to golang/tools that referenced this issue Dec 17, 2015

go/loader: use build.AllowVendor during Import on Go 1.6 and later
Makes programs like ssadump work on packages using vendored code,
for example net/http.

For golang/go#12278.
Depends on CL 17726 in main repository.

Change-Id: Ibabf564e397044a0f449087124dd96161081baaf
Reviewed-on: https://go-review.googlesource.com/17727
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-by: Alan Donovan <adonovan@google.com>
@FiloSottile

This comment has been minimized.

Copy link
Member

commented Dec 18, 2015

Many tools now work out of the box just rebuilding them with 1.6, thanks!

gopherbot pushed a commit to golang/tools that referenced this issue Feb 18, 2016

imports: add support for vendor directories
Editor modes that invoke the goimports command on temporary copies
of actual source files will need to invoke goimports -srcdir now to say
where the real source directory is. Otherwise goimports will not consider
vendored or internal packages when looking for new imports.

In lieu of a test for cmd/goimports (because it has no tests),
a command transcript:

	$ cd /tmp
	$ cat x.go
	package p
	var _ = hpack.HuffmanDecode
	$

	$ GOPATH= goimports < x.go
	package p

	var _ = hpack.HuffmanDecode
	$ GOPATH= goimports x.go
	package p

	var _ = hpack.HuffmanDecode
	$

But with the new flag:

	$ GOPATH= goimports -srcdir $GOROOT/src/math < x.go
	package p

	import "golang.org/x/net/http2/hpack"

	var _ = hpack.HuffmanDecode
	$ GOPATH= goimports -srcdir $GOROOT/src/math x.go
	package p

	import "golang.org/x/net/http2/hpack"

	var _ = hpack.HuffmanDecode
	$

The tests in this CL and the above transcript assume that
$GOROOT/src/vendor/golang.org/x/net/http2/hpack exists.
It did in 40a26c9, but it does not today.
It will again soon (once Go 1.7 opens).

For golang/go#12278 (original request).

Change-Id: I27b136041f54edcde4bf474215b48ebb0417f34d
Reviewed-on: https://go-review.googlesource.com/17728
Run-TryBot: Russ Cox <rsc@golang.org>
Reviewed-by: Andrew Gerrand <adg@golang.org>

ashwanthkumar added a commit to ashwanthkumar/marathon-alerts that referenced this issue Mar 7, 2016

@golang golang locked and limited conversation to collaborators Dec 29, 2016

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.