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

Feature to conditionally include bindgen dependency #489

Closed
hug-dev opened this issue Oct 8, 2020 · 4 comments · Fixed by #499
Closed

Feature to conditionally include bindgen dependency #489

hug-dev opened this issue Oct 8, 2020 · 4 comments · Fixed by #499

Comments

@hug-dev
Copy link
Contributor

hug-dev commented Oct 8, 2020

Hello 👋

We are facing a problem similar than this one, in our dependency tree we have multiple versions of bindgen and that fails to compile 😢

Since you are already committing the bindings in tree, for the Linux on x86 and Aarch64 targets, bindgen is not needed for those targets, if the bindings do not need to be updated with the UPDATE_BIND env var.

Because of that, I believe that the bindgen dependency (and maybe more) could be removed from grpc-sys with a special feature. That would make compilation much faster and fix those failed to select a version for clang-sys issues!

@BusyJay
Copy link
Member

BusyJay commented Oct 8, 2020

Good idea! Would you like to send a PR to implement it?

@BusyJay
Copy link
Member

BusyJay commented Oct 8, 2020

Actually I'm not sure if it's going to work as what you expect, because Cargo.lock is computed for all platforms.

@hug-dev
Copy link
Contributor Author

hug-dev commented Oct 8, 2020

Cool, I will see if I can do it in the next few days 😄 We can discuss the implementation there.

I basically added a feature activated by default "new-bindings" which will include bindgen. I put all bindgen related stuff in bindgen_grpc and conditionally compile and run this function with the new-bindings cfg-gate. The bad thing currently is that the bindings will always be generated for Aarch64 and x86 if this feature is set. I can re-factor further to have the same behaviour as before.

On the results side, there is around 45 seconds won (on my machine).

With the default feature:

   Compiling proc-macro2 v1.0.24
   Compiling memchr v2.3.3
   Compiling unicode-xid v0.2.1
   Compiling cc v1.0.60
   Compiling libc v0.2.79
   Compiling version_check v0.1.5
   Compiling glob v0.3.0
   Compiling syn v1.0.42
   Compiling cfg-if v0.1.10
   Compiling pkg-config v0.3.18
   Compiling pin-project-internal v0.4.26
   Compiling bitflags v1.2.1
   Compiling lazy_static v1.4.0
   Compiling proc-macro-nested v0.1.6
   Compiling bindgen v0.51.1
   Compiling regex-syntax v0.6.18
   Compiling same-file v1.0.6
   Compiling shlex v0.1.1
   Compiling futures-core v0.3.6
   Compiling once_cell v1.4.1
   Compiling peeking_take_while v0.1.2
   Compiling futures-sink v0.3.6
   Compiling proc-macro-hack v0.5.18
   Compiling rustc-hash v1.1.0
   Compiling futures-io v0.3.6
   Compiling slab v0.4.2
   Compiling pin-utils v0.1.0
   Compiling protobuf v2.18.0
   Compiling log v0.4.11
   Compiling smallvec v1.4.2
   Compiling scopeguard v1.1.0
   Compiling thread_local v1.0.1
   Compiling nom v4.2.3
   Compiling cmake v0.1.44
   Compiling walkdir v2.3.1
   Compiling clang-sys v0.28.1
   Compiling futures-task v0.3.6
   Compiling futures-channel v0.3.6
   Compiling lock_api v0.3.4
   Compiling aho-corasick v0.7.13
   Compiling libloading v0.5.2
   Compiling libz-sys v1.1.2
   Compiling quote v1.0.7
   Compiling parking_lot_core v0.7.2
   Compiling parking_lot v0.10.2
   Compiling cexpr v0.3.6
   Compiling regex v1.3.9
   Compiling futures-macro v0.3.6
   Compiling pin-project v0.4.26
   Compiling futures-util v0.3.6
   Compiling grpcio-sys v0.6.0 (/home/hugdev01/dev/grpc-rs/grpc-sys)
   Compiling futures-executor v0.3.6
   Compiling futures v0.3.6
warning: couldn't execute `llvm-config --prefix` (error: No such file or directory (os error 2))
warning: set the LLVM_CONFIG_PATH environment variable to a valid `llvm-config` executable
   Compiling grpcio v0.6.0 (/home/hugdev/dev/grpc-rs)
    Finished dev [unoptimized + debuginfo] target(s) in 1m 39s

Without:

   Compiling proc-macro2 v1.0.24
   Compiling unicode-xid v0.2.1
   Compiling syn v1.0.42
   Compiling cc v1.0.60
   Compiling pkg-config v0.3.18
   Compiling pin-project-internal v0.4.26
   Compiling libc v0.2.79
   Compiling memchr v2.3.3
   Compiling proc-macro-nested v0.1.6
   Compiling futures-sink v0.3.6
   Compiling proc-macro-hack v0.5.18
   Compiling once_cell v1.4.1
   Compiling futures-core v0.3.6
   Compiling cfg-if v0.1.10
   Compiling pin-utils v0.1.0
   Compiling futures-io v0.3.6
   Compiling slab v0.4.2
   Compiling smallvec v1.4.2
   Compiling log v0.4.11
   Compiling scopeguard v1.1.0
   Compiling futures-task v0.3.6
   Compiling futures-channel v0.3.6
   Compiling cmake v0.1.44
   Compiling lock_api v0.3.4
   Compiling quote v1.0.7
   Compiling libz-sys v1.1.2
   Compiling grpcio-sys v0.6.0 (/home/hugdev01/dev/grpc-rs/grpc-sys)
   Compiling parking_lot_core v0.7.2
   Compiling parking_lot v0.10.2
   Compiling futures-macro v0.3.6
   Compiling pin-project v0.4.26
   Compiling futures-util v0.3.6
   Compiling futures-executor v0.3.6
   Compiling futures v0.3.6
   Compiling grpcio v0.6.0 (/home/hugdev01/dev/grpc-rs)
    Finished dev [unoptimized + debuginfo] target(s) in 56.82s

hug-dev added a commit to hug-dev/grpc-rs that referenced this issue Nov 19, 2020
Fix tikv#489

Add the `use-bindgen` feature which is activated by default.

If
disabled, then the previously generated bindings will be used instead of
generating new ones. It will fail compilation if this feature is
disabled for the non supported targets.

If enabled, the behaviour is the same as before.

Signed-off-by: Hugues de Valon <[email protected]>
@hug-dev
Copy link
Contributor Author

hug-dev commented Nov 19, 2020

Just sent a PR for this! Tell me what you think 🤓

BusyJay pushed a commit that referenced this issue Dec 28, 2020
Fix #489

Add the `use-bindgen` feature which is activated by default.

If disabled, then the previously generated bindings will be used instead
of generating new ones. It will fail compilation if this feature is
disabled for the non supported targets.

If enabled, the behaviour is the same as before.

Signed-off-by: Hugues de Valon <[email protected]>
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 a pull request may close this issue.

2 participants