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

proposal: x/tools/cmd/goimports: support searching for Go packages within a Bazel WORKSPACE #18441

Open
mum4k opened this issue Dec 28, 2016 · 8 comments

Comments

@mum4k
Copy link

commented Dec 28, 2016

This feature request affects the functionality of golang/tools/cmd/goimports.

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

go version go1.7.3

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

linux/amd64

Background

  1. This is not a bug report, but a feature request.
  2. I would like to contribute and implement this feature.
  3. The purpose of filing this issue is to indicate that I would like to work on this and to agree on the implementation.

The feature

Google released Bazel. Bazel's supports for Go is via the Skylark go_rules.

Bazel organizes all source code under a WORKSPACE directory, say /some/path/to/workspace further referred to as the bazel_workspace. The go_rules require the user to set a WORPSKACE-wide variable go_prefix which is the global prefix used to fully qualify all Go import targets. This may be yourcompany further referred to as the bazel_go_prefix.

With this setup a package named foo located in file /some/path/to/workspace/subdir/foo/foo.go should be imported as:

<bazel_go_prefix>/subdir/foo

With the above mentioned bazel_go_prefix becomes:

"yourcompany/subdir/foo"

The desired feature is to enable goimports to find packages under a Bazel WORKSPACE and to add correct import paths for these packages.

Proposed implementation

I would add two new flags into the goimports main:

  1. -bazel_workspace for setting the absolute path to the Bazel's WORKSPACE .
  2. -bazel_go_prefix for setting the go prefix.

The code in golang/tools/imports/fix.go would be modified as follows:

  1. Two new global vars will be added to hold the values of the new flags.
  2. A new function scanBazelWorkspace() will be added, that when called would scan the Bazel WORKSPACE similarly to how the Go directories are scanned today, but with Bazel specific path manipulation.
  3. The scanBazelWorkspace() function would be called from findImportGoPath() if both of the new flags were set.
  4. The scanBazelWorkspace() function would be called concurrently with scanGoRoot() and scanGoPath().
  5. Sorting of the discovered candidates would be modified, if the two new flags were specified, the import paths within the Bazel WORKSPACE would be preferred, the rest of the candidates would be sorted as today - based on the length, preferring the shortest.

I have a prototype that I tested this locally. Please let me know if you agree with the proposal above, in which case I will start the code review process.

Alternatively feedback or suggestions are welcome.

@mum4k mum4k changed the title goimports should support searching for Go packages within a Bazel WORKSPACE x/tools/cmd/goimports: support searching for Go packages within a Bazel WORKSPACE Dec 28, 2016

@bradfitz

This comment has been minimized.

Copy link
Member

commented Dec 28, 2016

I'd prefer to wait and figure out what the full story is between $GOPATH, Bazel, and Go 1.9 vendoring work before moving forward with ad hoc changes like this one.

I worry that changing goimports would effectively encourage Bazel layout (at least a bit) and fragment the ecosystem in two mutually inoperable halves, killing happy network effects.

I'm more interested in figuring out the long-term story instead of the short-term "Proposed implementation" details.

/cc @rsc @spf13 @broady

@bradfitz bradfitz changed the title x/tools/cmd/goimports: support searching for Go packages within a Bazel WORKSPACE proposal: x/tools/cmd/goimports: support searching for Go packages within a Bazel WORKSPACE Dec 28, 2016

@mum4k

This comment has been minimized.

Copy link
Author

commented Dec 28, 2016

Thanks, I wasn't aware that we don't want to encourage the Bazel layout.

Since you mention that the proposed implementation would be a "shot-term" fix, does this mean that there is some other plan for closing the gap between Golang and Bazel? I wasn't able to find any details so if you could point me to it or share some, that would help.

@bradfitz

This comment has been minimized.

Copy link
Member

commented Dec 28, 2016

There is a plan to figure out the plan, at least.

@rsc rsc added this to the Proposal milestone Jan 4, 2017

@broady

This comment has been minimized.

Copy link
Member

commented Jan 6, 2017

/cc @alandonovan, who was looking into Go and Bazel recently.

@rsc rsc added the Proposal-Hold label Jan 23, 2017

@gonzojive

This comment has been minimized.

Copy link

commented Feb 8, 2018

Any developments since 1/2017?

@nemith

This comment has been minimized.

Copy link
Contributor

commented Dec 7, 2018

Does this change now with golang.org/x/tools/go/package?

@ianthehat

This comment has been minimized.

Copy link

commented Dec 7, 2018

Yes.

There is currently a change in review that makes goimports work in terms of go/packages

go/packages will have a bazel "driver" that knows how to understand the layout by asking bazel itself to tell it about the rules. This is still a work in progress with no solid timeline on when it will be done, but when it is all tools that have been converted to go/packages will start working in bazel workspaces.

@robfig

This comment has been minimized.

Copy link
Contributor

commented Mar 12, 2019

@ianthehat do you have a link to a tracking issue for the go/packages bazel driver so I can follow along?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants
You can’t perform that action at this time.