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

Support specifying custom package name #16

Closed
damienmg opened this issue Apr 26, 2016 · 11 comments
Closed

Support specifying custom package name #16

damienmg opened this issue Apr 26, 2016 · 11 comments

Comments

@damienmg
Copy link
Contributor

See bazelbuild/bazel#828

@yugui
Copy link
Member

yugui commented Apr 26, 2016

IIUC, the main problem described in bazelbuild/bazel#828 has gone because now label names can contain slash.
However there are other several patterns which potentially require custom package names.

I'll summarize the patterns and remaining problems tomorrow or day after tomorrow.

ivucica added a commit to ivucica/rules_go that referenced this issue May 1, 2016
@ivucica
Copy link

ivucica commented May 1, 2016

Preliminary work in f32a39c. It suits my immediate hacking needs, but I'll try to come up with something that's actually properly committable (not to mention with the correct git author field).

@damienmg I didn't check yet, but I'll assume this is maintained primarily externally? And also, given that there's no project on Gerrit, I'll assume that a Github PR is the correct way to contribute?

@yugui
Copy link
Member

yugui commented May 1, 2016

Sorry for being a bit late.


It'd be better to discuss use cases of customization.

In my understanding, basically users need to customize import paths when they import third-party libraries. So before discussing solutions to customize import paths, I'd like to summarize several possible patterns of importing third-party libraries.
Next, I'd like to propose a solution.

Patterns of Bazelization of third-party libraries

TL;TR. It shows that the main use case of import path customization is the case that the user uses new_git_repository to import third-party libraries.

Pattern 1: Add a BUILD file into each third-party library.

This is the primary way which rules_go assumes.

example: https://github.com/yugui/rules_go-vendor-example/tree/sutree-merge

Pro

  • It works perfectly when users manage vendored subtree with git merge -s subtree or something.

Cons

  • It requires users to manually add a file into the library tree.
    So it does not work when they use git submodule, git subtree or some library management tools like Godep, Gom or Glide.

  • It is hard to manage the third-party library if it has its own WORKSPACE and BUILD files.
    e.g. yugui/rules_go-vendor-example@fdf5a74

    The user have to modify the existing BUILD files in the third-party library to adjust references to package names whenever they upgrade the library. It often causes merge-conflict, so it is hard to upgrade the library.

Pattern 2: Add a single //vendor:BUILD.

This was the main problem in bazelbuild/bazel#828.
It did not work fine when bazelbuild/bazel#828 was discussed but now it works because target labels can contain slash.

example:

Pro

  • It does not require users to manually modify the third-party tree.
    So it works even if the third-party library is managed with git submodule, git subtree or a libary management tool.

Cons

  • It would get difficult to manage the single //vendor:BUILD if the project depends on a large number of third-party libraries.
  • The user have to manually keep the //vendor:BUILD up-to-date even if the third-party library already supports
    Bazel and it already has BUILD files.
    This is a bit easier than solving merge-conflicts, however it is also easier to overlook something.

Pattern 3: Use new_git_repository or similar workspace rules

This is another proper way to manage third-party dependencies in Bazel.

example: https://github.com/yugui/rules_go-vendor-example/tree/new_git_repository

Pros

  • When the project does not use vendor directory, it requires minimum changes to support Bazel.
  • It works fine even if the third-party library already has BUILD files.
    Just use git_repository instead of new_git_repository.

Cons

  • It is far from the standard convention of Golang.

  • It does not work today because go_library and other rules do not recognize vendoring in external/.
    So in the example, Bazel can resolve the third party dependency @glog:go_default_library but go compiler fails to resolve the import path github.com/golang/glog.

    This is the case in which users need import path customization.

Summary of the patterns

Each of the possible patterns has both pros and cons. So it is reasonable to support all of the patterns.
And the last pattern does require import path customization.


Solution for the issue in the third pattern

i.e. in the example of https://github.com/yugui/rules_go-vendor-example/tree/new_git_repository,
go_library rule should emit

-importmap=github.com/golang/glog=external/glog/go_default_library

because go_prefix attribute of @glog//:go_default_library points @glog//:go_prefix, which is github.com/golang/glog.
Then //:example can resolve the import path github.com/golang/glog.

Pros:

  • Does not require a new attribute or new rules
  • Does not give too much flexibility of import path customization.
    Even with some amount of cusotmization, golang import path should still reflect package structure for ease of understanding
    and consistency to the standard convention of Golang.
    In this perspective, this approach is flexible enough to support the third pattern of vendoring but does not allow users to completely decouple package structure from import path.

Cons:

  • It would not be flexible enough if there is a use case which really need to completely decouple package structure from golang import path. We need a real use case if this is really a problem.
  • Some possible implementation difficulties?

@ivucica
Copy link

ivucica commented May 1, 2016

I love the overview!

  • I'd say that Pattern 2, con 2, doesn't really apply; if an external project already uses Bazel, I'm probably happy to use their rules.
  • Pattern 2, con 1 is solvable by autogenerating BUILD files for projects that are using go build and aren't providing BUILD files (see Create "glaze" BUILD file generator for Go #15). Unfortunately, I didn't yet come up with satisfying output from my attempt at tackling this.

I agree that the need for total customization of package names, esp. in relation to pattern 2, has dropped off significantly when slashes in target names became possible. I'm trying this out now and the porting over to Bazel rules is going extremely smoothly.


In addition to the commentary above, I have run into additional use case in my small project from bazelbuild/bazel#828: generated proto->go files.

To make building easier with go {get,build,install}, I'm storing generated Go files in scheduler/api/*.go. However, when building with Bazel, I want to rebuild the protos. (I'm currently using three clumsy genrule()s for rebuilding the regular Go source, grpc-gateway Go source and letmegrpc Go source.)

genrule()'s outputs would conflict with existing files -- Bazel doesn't approve of having outputs matching existing files. I didn't dig deep enough to see if I can influence the output filenames, but I assumed I can't.

Current workaround for the problem:

  • store proto as //scheduler:scheduler.proto
  • place genrule()s into BUILD for //scheduler instead of //scheduler/api, making it spew out files such as bazel-genfiles/scheduler/scheduler.gw.pb.go
  • write a go_library(name='api') in //scheduler's BUILD with import path my.project/import/path/scheduler/api

This way, go build works out of the box, but Bazel is still happy to regenerate the files.

@yugui
Copy link
Member

yugui commented May 2, 2016

I'd say that Pattern 2, con 2, doesn't really apply; if an external project already uses Bazel, I'm probably happy to use their rules.

You can reuse the rule if the external project is simple enough. However, similarly to the con 2 of the pattern 1, you cannot reuse if the external project have several Bazel packages.
So there's no option to avoid the pattern 2 and its con 2 if your project imports external projects with git submodule and the external project have several BUILD files.

In addition to the commentary above, I have run into additional use case in my small project from bazelbuild/bazel#828: generated proto->go files.

That's an interesting use case. But I couldn't get your point about how import path customization would help this use case.

@damienmg
Copy link
Contributor Author

damienmg commented May 2, 2016

FWIW I would prefer pattern 3 because it is more the Bazel philosophy. We could create a custom go_vendor() rule for the WORKSPACE file that would create a good skylark kind with a provider for the list of import map. That would work just fine. If the rules is correctly set-up then it could actually cover both pattern 1 and 3.

@ivucica
Copy link

ivucica commented May 2, 2016

@damienmg I like pattern 3 as well -- but why would I go with pattern 2 (git submodules in //vendor) then?

  • I like having explicit dependencies when using go build.
  • I like checking out code once - that's a bit harder with {,new_}git_repository(). Bazel likes to download new copies of the code when its workspace goes away. I'm happier if I provide the code myself, at least during development.
  • I like being able to easily touch code if I want to contribute upstream; unless I use {,new_}local_repository(), that becomes harder. (That's why I'm keeping even Bazel rules for Go in //vendor/github.com/bazelbuild/bazel_rules.)

@yugui Ah, I missed the "workspaces are not importable" problem.

Regarding my use of renaming, I'll try to describe without going all the way to provide a public demo.

  • //scheduler/api:scheduler.pb.go et al are committed and distributed in Git. This is to help depending on the Bazel module when using go build, go install etc.
    • Their location makes these files part of the my.project/name/scheduler/api Go package.
  • I've attempted to have //scheduler/api:BUILD contain the genrule() to spit out protos.
    • Input is //scheduler/api:scheduler.proto, of course.
    • This didn't work out well: the files already exist, and the Bazel output names match them. This is not allowed.
    • This was back around Bazel 0.1.4 or 0.1.5 but I do not expect things to have changed.
  • I've instead put the proto as //scheduler:scheduler.proto, and the genrule()s in the accompanying BUILD file.
    • This works, but now the files are part of the my.project/name/scheduler package.
    • This is not expected by the rest of the Go project.
    • Thus, even though the outputs of genrule()s seem to live in //scheduler package, equivalent to Go's my.project/name/scheduler package as detected by go_library(), they need to be renamed and exposed to the rest of the Go project as my.project/name/scheduler/api.

Sorry I wasn't clearer before; does this make more sense?

@yugui
Copy link
Member

yugui commented May 30, 2016

@damienmg
Do you mean that go_vendor repository_rule runs something like glaze and the automatic configuration should have an appropriately configured import map?
It sounds good for me.

@ivucica
Ah, thank you. Now I understand the problem.
Custom package name can be a good workaround for the problem, but it am wondering if this problem is really Go-specific.

More generically, can we say that existing repositories sometimes contains generated files because those files are hard to regenerate but in Bazel it is easier to regenerate them so it is better to let Bazel regenerate?
If so, the straightforward and more generic solution would be let Bazel ignore specific files in the workspace. Is it realistic to have .bazelignore file or something, @damienmg ?

In my understanding, existing repositories tend to contain generated files because treditional build tools were not good at installing uncommon code generators. e.g.

  • Release tarball of Ruby contains files generated by flex, bison and autoconf.
    • It is because they want to allow users to build Ruby only with make, sed and C compiler.
  • It happens often with protobuf because protoc is less common than cc or javac.
  • It happens very often in Go projects because the convention of Go expects that you can build the project just with "go get" backed with go toolchain and C compiler.

On the other hand, it is easy for Bazel to write a skylark rule which installs/builds flex, bison, protoc or other code generators. So in Bazel, it seems to be preferred to generate avoid using generated files checked into the repository.

For me, this difference between traditional build tools and Bazel seems to be the cause of the problem. So I am wondering if we can have a more generic solution.

@damienmg
Copy link
Contributor Author

@yugui: yes :)

yugui added a commit that referenced this issue May 30, 2016
Uses go_prefix attr of individual dependencies to calculate their
importpaths.  This allows users to use Bazel's external dependency
resolution -- {,new_}git_repository rules -- to manage Go libraries.

c.f. #16 (comment)
yugui added a commit that referenced this issue May 30, 2016
Uses go_prefix attr of individual dependencies to calculate their
importpaths.  This allows users to use Bazel's external dependency
resolution -- {,new_}git_repository rules -- to manage Go libraries.

c.f. #16 (comment)
@yugui
Copy link
Member

yugui commented May 30, 2016

In #30, I implemented the solution I proposed. It allows us to deal with the case 3 and it is also useful for the case 1.

As @damienmg suggested, it is better to define a repository rule go_vendor which generates something similar to WORKSPACE#L6-19

Unfortunately #30 does not solve @ivucica's use case. But #30 is still useful as a foundation to support his use case if necessary.

yugui added a commit that referenced this issue May 30, 2016
Uses go_prefix attr of individual dependencies to calculate their
importpaths.  This allows users to use Bazel's external dependency
resolution -- {,new_}git_repository rules -- to manage Go libraries.

c.f. #16 (comment)
damienmg pushed a commit that referenced this issue Jun 6, 2016
* Github-flavored markdown now supports bzl format

* Fix typo

* Follow e08ee96 in documents

* Wrap lines to keep them shorter than 80 characters

* Now bazel accepts "github.com" as a directory name

c.f. abe7374 and
#16 (comment)
yugui added a commit that referenced this issue Aug 30, 2016
yugui added a commit that referenced this issue Aug 30, 2016
@ianthehat
Copy link
Contributor

I think everything in here is now covered by the explicit import_path and flat build file solutions.

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

No branches or pull requests

4 participants