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

RFC: Use precompiled libprotobuf.a and libprotobuf-lite.a for macOS x86_64 host/exec configuration #83

Closed
wants to merge 1 commit into from

Conversation

thii
Copy link
Member

@thii thii commented Feb 5, 2021

Inspired by #36, this tries to use prebuilt binaries of libprotobuf.a and libprotobuf-lite.a to reduce the compilation overhead of targets depending on @com_google_protobuf//:protobuf when building on macOS x86_64. Since the protobuf repo doesn't provide prebuilt libprotobuf.a and libprotobuf-lite.a, this uses the binaries from Homebrew.

This can be useful for rules_swift, as the compilation of its persistent worker is depending on protobuf, which can take a very long time, especially for small/non-cache projects.

Open question: Should we add a flag and make this an opt-in feature? Since people may want to compile everything from source for the best compatibility.

This tries to use prebuilt binaries of libprotobuf.a and
libprotobuf-lite.a to reduce the compilation overhead of targets
depending on `@com_google_protobuf//:protobuf` when building on macOS
x86_64. Since the protobuf repo doesn't provide prebuilt libprotobuf.a
and libprotobuf-lite.a, this uses the binaries from Homebrew.

This can be useful for rules_swift, as the compilation of its persistent
worker is depending on protobuf, which can take a very long time,
especially for small/non-cache projects.
@thii
Copy link
Member Author

thii commented Feb 15, 2021

Bump.

Also cc @Yannic since you reviewed the referenced PR.

@Yannic
Copy link
Collaborator

Yannic commented Feb 23, 2021

I'm concerned that it becomes increasingly hard and error-prone for us to upgrade protobuf if we keep doing these kind of things.

-1 from me since (at least this PR) is only helping a small fraction of users and there is a reasonable workaround (define a proto_lang_toolchain for C++ in another workspace and pass --proto_toolchain_for_cc=//path/to:my_cc_toolchain to Bazel)

Copy link
Collaborator

@comius comius left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The maintenance burden this introduces is quite high: need to mirror, upload all os + architecture of protobuf, verify homebrew binaries (or replace them with a trusted source); repeat everything on version upgrade.

I wouldn't have a problem if there was a general+automatic mechanism to do this. Otherwise I would rather move away from precompiled binaries wherever possible and work on mechanisms that would solve the problem more generally - caching services, whatever.

With this PR only one OS is handled. Protoc PR handled all OSes, and used a source of archives from github, which we can more easily trust that homebrew. I believe protoc PR also supports the use case to compile for languages that wouldn't otherwise need cc compiler - i.e removes the dependency on cc compiler.

@thii
Copy link
Member Author

thii commented Mar 17, 2021

Agreed with the comments. I'll drop this for now.

@Yannic Thanks for the tips on the --proto_lang_toolchain flag.

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

Successfully merging this pull request may close these issues.

None yet

3 participants