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

Deprecate and remove go_prefix #721

Closed
jayconrod opened this issue Aug 14, 2017 · 6 comments
Closed

Deprecate and remove go_prefix #721

jayconrod opened this issue Aug 14, 2017 · 6 comments

Comments

@jayconrod
Copy link
Contributor

go_prefix is used to translate Bazel labels (for example, //foo/bar:go_default_library) to Go import paths (example.com/repo/foo/bar) and vice versa. There are a number of problems with go_prefix, and we'd like to stop using it and eventually stop supporting it.

  • Go rules implicitly depend on //:go_prefix. This means there must be a go_prefix defined in the repository root package, even if there are no other Go rules in that package.
  • This does not work well for Go subtrees that are part of larger repositories. The actual import path of libraries within the repository may not correspond to <go_prefix>/<subtree>/<library>.
  • This does not work for vendoring. We have special cases for vendoring in several places.
  • It's confusing and unexpected that go_prefix is required for go_binary. For binary-only repositories, it shouldn't be required at all.

We would like to move away from using go_prefix. go_library already has an importpath attribute, which allows the import path to be set explicitly. This is currently optional, but it may be required in the future.

We will deprecate go_prefix slowly in several steps to avoid breaking people.

  • Gazelle will add importpath attributes to new and existing go_library rules. These import paths will match the import paths that already would be computed, based on the current go_prefix and the relative path within the repository.
  • go_prefix will no longer be mandatory for go_library rules that specify importpath. We should be able to accomplish this with computed dependencies.
  • Gazelle will no longer generate go_prefix rules. When Gazelle is run with the "fix" command, it will remove existing go_prefix rules.
  • At some point in the distant future, we may remove go_prefix altogether (this might not actually happen, but it's the plan of record).
jayconrod pushed a commit to jayconrod/rules_go that referenced this issue Aug 16, 2017
go_prefix is used to generate Go import paths from Bazel labels. Until
now, it has been mandatory, since there's an implicit dependency from
all go_library, go_binary, and go_test rules on //:go_prefix. With
this change, that dependency is not present when the importpath
attribute is specified. This means that if all of the Go rules in a
repository specify importpath, there's no need to define a go_prefix
rule in the repository root.

Fixes bazel-contrib#720
Related bazel-contrib#721
jayconrod added a commit that referenced this issue Aug 17, 2017
go_prefix is used to generate Go import paths from Bazel labels. Until
now, it has been mandatory, since there's an implicit dependency from
all go_library, go_binary, and go_test rules on //:go_prefix. With
this change, that dependency is not present when the importpath
attribute is specified. This means that if all of the Go rules in a
repository specify importpath, there's no need to define a go_prefix
rule in the repository root.

Fixes #720
Related #721
jayconrod pushed a commit to jayconrod/rules_go that referenced this issue Aug 29, 2017
* go_prefix is no longer generated. In a future change, existing
  go_prefix rules will be removed in fix mode.
* All go_library, go_test, and go_binary rules are generated with an
  importpath attribute.
* importpath is now a mergeable attribute; Gazelle will replace
  existing importpath attributes unless they have a keep comment. This
  helps keep libraries up to date after they are moved to a new
  location.

Related bazel-contrib#721
jayconrod added a commit that referenced this issue Aug 30, 2017
* go_prefix is no longer generated. In a future change, existing
  go_prefix rules will be removed in fix mode.
* All go_library, go_test, and go_binary rules are generated with an
  importpath attribute.
* importpath is now a mergeable attribute; Gazelle will replace
  existing importpath attributes unless they have a keep comment. This
  helps keep libraries up to date after they are moved to a new
  location.

Related #721
@nictuku
Copy link

nictuku commented Nov 23, 2017

Not sure if it's related, but do we need importpath on go_binary and go_image? One currently has to set either //:go_prefix or importpath when calling that rule. As a user new to open-source Bazel, this seems confusing.

Does this Issue about go_prefix also imply that the importpath requirement will also eventually be removed? That would be nice.

@jayconrod
Copy link
Contributor Author

At the moment, importpath is needed for go_binary if //:go_prefix does not exist. I can't comment on go_image.

Internally, a go_binary rule describes a package that needs to be compiled, just like go_library. The importpath determines the filename for this package. The difference is that go_binary gets linked and doesn't get imported by other rules, so the filename doesn't matter much for compilation.

The one place it is used publicly is the experimental go_path rule, which pulls all the sources for a set of rules into a GOPATH-like directory tree. go_path uses importpath to determine where to put sources in that tree. There are some experimental rules based on go_path, like go_vet. It's likely that we'll need to run any tool that requires a GOPATH on top of something like this, so the importpath attribute will continue to be important.

@mattmoor
Copy link

go_image is a pass-through to go_binary (at the version the user imports) so it's requirements shouldn't be any different.

@nictuku
Copy link

nictuku commented Nov 27, 2017

If go_prefix is going away, all go_binary will have importpath and that's confusing for a Go binary. IIRC, internally at Google, importpath was not required for go_binary.

Can we use a convention for the path of go_binary that can stand-in when importpath isn't set?

@ianthehat
Copy link
Contributor

gazelle writes the importpath into the rule for you, so it will always be set anyway.
We are trying to make sure that heuristics and conventions live in gazelle, not the rules.
The idea is that the build files tell you everything you need to know, rather than making you guess what will happen, and that a little verbosity is fine so long as you don't have to update it by hand.

ianthehat added a commit to ianthehat/rules_go that referenced this issue Jan 3, 2018
Infer test import path correctly from deps
Remove importpath from all go_binary and go_test rules
Add importpath to all go_library rules
Delete go_prefix occurences

Progress on bazel-contrib#721
ianthehat added a commit that referenced this issue Jan 4, 2018
* Fixup importpath

Infer test import path correctly from deps
Remove importpath from all go_binary and go_test rules
Add importpath to all go_library rules
Delete go_prefix occurences

Progress on #721

* Review fixes
@jayconrod
Copy link
Contributor Author

Closing old issues.

go_prefix is gone. go_binary does not require importpath, but go_library does.

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

No branches or pull requests

4 participants