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

Split the compiler from the Bazel rules #5402

Closed
hlopko opened this issue Nov 27, 2018 · 12 comments
Closed

Split the compiler from the Bazel rules #5402

hlopko opened this issue Nov 27, 2018 · 12 comments
Assignees
Labels

Comments

@hlopko
Copy link
Contributor

hlopko commented Nov 27, 2018

Hello protobuf team,

we're experiencing a lot of friction when using Bazel and protobuf, and I think it's caused by the fact that proto rules and the compiler live in the same repository and are always released together. If we could split them, I'd expect these benefits:

  1. rules can be released more often (Bazel is moving quite quickly and we expect incompatible changes almost every Bazel release, which is currently every month). Almost every ruleset of the ecosystem depends on protobuf, so not releasing protobuf pretty much blocks everybody.
  2. rules can be taught to use system installed version of the protobuf compiler. This is currently something Tensorflow struggles with (perfinion/tensorflow@5e62ff1).

Maybe there are more, I'm in the brainstorming phase :) Wdyt?

@lberki @dslomov @perfinion @laurentlb @gunan

@acozzette
Copy link
Member

Are you thinking of having the proto rules released as part of Bazel itself?

@hlopko
Copy link
Contributor Author

hlopko commented Nov 27, 2018

Not necessarily (I personally would prefer having fewer rules released with Bazel).

@dslomov
Copy link
Contributor

dslomov commented Nov 27, 2018

Are you thinking of having the proto rules released as part of Bazel itself?

Not as part of Bazel itself, but we would like to make sure that protobuf rules can follow released Bazel closely (i.e. follow Managing breaking changes in Bazel), and protobuf has long release cycles (for a recent problem this has caused, see bazelbuild/bazel#6675 where the protobuf at head was updated to catch up with incompatible change some time ago, but never released)

@perfinion
Copy link

Thanks Marcel for opening this!

Are you thinking of having the proto rules released as part of Bazel itself?

I was thinking the rules should be installed on the system with protoc & libs and similar to how the pkg-config file or FindProtobuf.cmake are installed.

I was discussing my TF/Bazel issues with Marcel but let me put a summary here for completeness. Basically I've been trying to unbundle all the dependencies from TensorFlow so it can better follow the guidelines for packaging on Linux distros. Unbundling most libraries is fairly easy cuz they only have a library. Protobuf has been comparatively problematic tho. I can point to my system protoc and libprotobuf etc, but I have to duplicate the protobuf.bzl rules into TF itself in order to be able to use those rules which is annoying for maintainability. I could download the entire protobuf source tarball but that's a bit extreme considering I only need one single file. I don't mean to pick on protobuf specifically, there are similar problems with other projects too its just that protobuf is so widely used that I've had the most issues with it so I think fixing the issues here would be a good guideline for other projects to follow.

I have opened the PR for unbundling protobuf that works in TF with now things are right now: tensorflow/tensorflow#24004 . I'll definitely be keeping an eye on this and update TF as things change in the future :).

@haberman
Copy link
Member

haberman commented Dec 5, 2018

The command-line options to protoc change very infrequently, so I don't see any need to couple the protobuf rules (https://github.com/protocolbuffers/protobuf/blob/master/protobuf.bzl) with protobuf releases.

On the other hand, the set of files in protobuf itself changes somewhat frequently. The PR referenced above (https://github.com/tensorflow/tensorflow/pull/24004/files) seems to attempt to enumerate all protobuf public headers. I this we should avoid this approach if possible. Perhaps we should ship with protobuf a BUILD file that exposes cc_library() rules for the system-installed protobuf.

This would be similar to how libraries using CMake ship a .cmake file that describes how to use the installed version of a library.

@hlopko
Copy link
Contributor Author

hlopko commented Dec 6, 2018

Results of offline chat:

  • we can separate the rules from the compiler
  • we will include a BUILD file listing all the binaries/libraries/headers with the compiler release.

Josh, can this be left for the protobuf team to take care of?

@haberman
Copy link
Member

haberman commented Dec 7, 2018

I just rediscovered the native protobuf rules for Bazel (proto_library(), cc_proto_library(), etc): https://blog.bazel.build/2017/02/27/protocol-buffers.html

@perfinion is there a reason you are using the rules from protobuf.bzl instead of these? Our rules are actually marked as internal-only, and something that should probably be migrated to the Bazel-native rules:

protobuf/protobuf.bzl

Lines 232 to 234 in 7b28271

NOTE: the rule is only an internal workaround to generate protos. The
interface may change and the rule may be removed when bazel has introduced
the native rule.

The Bazel native rules are more modern and should be easier to use. I think we should guide people towards these, and fill in any missing functionality that might cause people to use protobuf.bzl instead. Maybe we can even remove protobuf's own use of protobuf.bzl.

@hlopko
Copy link
Contributor Author

hlopko commented Dec 14, 2018

@dslomov @lberki @gunan Are we all on board with directing Bazel users away from protobuf/protobuf.bzl? Towards native Bazel rules? Towards new rules in bazelbuild org? Do I remember correctly that @dslomov and @haberman discussed this last week? What was the result of the conversation?

Thank you all for participating! It's great to see a will to move forward from the protobuf rules chaos we have right now.

@dslomov
Copy link
Contributor

dslomov commented Dec 14, 2018

@haberman and I discussed this, and I think the right move would be to steer people towards native Bazel rules (and wrap them in Starlark and eventually migrate)

@perfinion
Copy link

TF wasnt using the native rules so I kept on not-using them. I will investigate switching things over tho, sounds like thats easiest for everyone. TF also just bumped the minimum bazel version to 0.18 so everyone should be on a new enough version now. :)

@hlopko
Copy link
Contributor Author

hlopko commented Mar 12, 2019

@perfinion Did you have time to check if TF can use native rules?

@haberman
Copy link
Member

@comius is driving a project to improve the Bazel rules https://docs.google.com/document/d/1l5JrFOxjU1USWXoc_KeBIjBoae5nkaL5AqhNiHLYeyI/edit

I agree that we should steer people towards rules_proto, and away from protobuf.bzl.

I'm closing this bug now as I don't believe there is anything further that is actionable from our side.

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

No branches or pull requests

6 participants