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

grpcio: pre-generate bindings for common targets #356

Merged
merged 40 commits into from
Aug 5, 2019
Merged

grpcio: pre-generate bindings for common targets #356

merged 40 commits into from
Aug 5, 2019

Conversation

hunterlxt
Copy link
Member

Building bindings at compile time introduces extra dependencies. And clang is hard to install and configure on some platform like centos 7. It will be very convenient that we can pre-generate bindings for common targets like x86_64-unknown-linux-gnu. For others, we can fallback to rust-bindgen at compile time.

grpc-sys/src/lib.rs Outdated Show resolved Hide resolved
@hunterlxt hunterlxt requested a review from BusyJay July 29, 2019 09:37
@hunterlxt
Copy link
Member Author

pre-bindings files could be different on different platforms and environments. Even in two computers in the same operating system environment, the order of the files is different. It may be difficult to use git diff to verify that the file has been modified. @BusyJay @nrc

@hunterlxt hunterlxt requested a review from nrc July 29, 2019 15:12
@hunterlxt
Copy link
Member Author

PLTA @BusyJay

.travis.yml Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
grpc-sys/build.rs Outdated Show resolved Hide resolved
grpc-sys/build.rs Outdated Show resolved Hide resolved
grpc-sys/src/lib.rs Outdated Show resolved Hide resolved
@nrc
Copy link
Contributor

nrc commented Aug 1, 2019

@BusyJay do you have thoughts on #356 (comment) ?

@BusyJay
Copy link
Member

BusyJay commented Aug 2, 2019

there seems more risk here than there since the protobuf code is at least multi-platform.

That's why the bindings are named according to the target platform. It's intended to only maintain limited platform.

I agree we should remove the pre-generated bindings when rust-bindgen supports bundled clang. For now, pre-generating seems a more suitable solution.

.travis.yml Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
grpc-sys/build.rs Outdated Show resolved Hide resolved
grpc-sys/build.rs Outdated Show resolved Hide resolved
@BusyJay
Copy link
Member

BusyJay commented Aug 5, 2019

Because clang is the default compiler on MacOS, so I drop the support. PTAL

grpc-sys/src/lib.rs Outdated Show resolved Hide resolved
disksing
disksing previously approved these changes Aug 5, 2019
grpc-sys/build.rs Outdated Show resolved Hide resolved
Copy link
Member

@Connor1996 Connor1996 left a comment

Choose a reason for hiding this comment

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

LGTM

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.

6 participants