grpc-sys: Fix build failure on uncommon linux build targets#644
grpc-sys: Fix build failure on uncommon linux build targets#644harryoooooooooo wants to merge 1 commit intotikv:masterfrom
Conversation
In the current implemntation, if the TARGET is not listed in build.rs ( e.g. x86_64-cros-linux-gnu) but target_os/target_arch matches (x86_64|aarch64)/(macos|linux), then the path $OUT_DIR/grpc-bindings.rs will be selected but actually not generated. This patch fixes the issue by using the same condition to detect the supported platforms; If supported, select pre-generated path, otherwise select OUT_DIR path and generate the bindings.
| _ => PathBuf::from(env::var("OUT_DIR").unwrap()).join("grpc-bindings.rs"), | ||
| let file_path: PathBuf; | ||
|
|
||
| #[cfg(not(any( |
There was a problem hiding this comment.
Instead of changing here, you should make the conditional check at L505~508 more accurate. And Cargo.toml should also be updated to include necessary dependencies.
There was a problem hiding this comment.
Instead of changing here, you should make the conditional check at L505~508 more accurate.
The point of this PR is simple: Make sure the binding file is generated if we're not going to use the pre-generated one. That is, if the OUT_DIR path is selected then we MUST generate the binding. Since you prefer changing the L505-508 then my understanding is:
let target = env::var("TARGET").unwrap();
let (file_path, need_update) = match target.as_str() {
"x86_64-unknown-linux-gnu"
| "x86_64-unknown-linux-musl"
| "aarch64-unknown-linux-musl"
| "aarch64-unknown-linux-gnu"
| "x86_64-apple-darwin"
| "aarch64-apple-darwin" => {
// ...
(PathBuf::from(...), cfg!(feature = "_gen-bindings"))
}
_ => (PathBuf::from(...), true),
};
if need_update {
// On some system (like Windows), stack size of main thread may
// be too small.
let f = file_path.clone();
// ...
What do you think?
There was a problem hiding this comment.
What I mean is bindings is pregenerated only on following targets:
"x86_64-unknown-linux-gnu"
| "x86_64-unknown-linux-musl"
| "aarch64-unknown-linux-musl"
| "aarch64-unknown-linux-gnu"
| "x86_64-apple-darwin"
| "aarch64-apple-darwin"
So what we need to do is figure out a more precise #[cfg(xxx)] that exactly matches those targets.
There was a problem hiding this comment.
Is it necessary to approach this in compile time, i.e., with #[cfg(xxx)] tag?
If not then I think my proposal already addressed this by cfg!(feature = "_gen-bindings").
There was a problem hiding this comment.
Is it necessary to approach this in compile time, i.e., with #[cfg(xxx)] tag?
Yes. The point of pre-generating bindings is to avoid downloading bindgen and depends on LLVM on common platforms.
|
Can we please finally merge this? |
|
@Eclextic what issue did you meet? Master branch has already changed the target match rules, so I think for uncommon targets, bindings should be generated at compile time already. |
|
I don't see any revalent changes after this PR so I believe the issue is still there. An uncommon linux target would enter neither of the checks below, and as a result there's no bindings.rs file. In my opinion the two checks must be complement (like what this PR does) to cover all cases. As per #discussion_r1602818903 the first one seems to be preferred, so I guess we shall chage the second one correspondingly. |
I mean I am gonna be honest. In hindsight I am kind of unsure if this is related at all, but whenever one cargo cleans and recompiles the project I get a cmake deprecation error on linux. I think this is technically unrelated to this issue and moreover related to #620. So yeah I am sorry. I'll continue posting there. |
3 participants
Hi owners, I'm Hsin-chen from Google. Recently we stepped into a build failure in our product, below is the failure reason.
After some investigation I believe the culprit is around the
config_binding_pathfunction: Our build target is not listed as a supported platform but the bindings are not generated either because our target_os is linux. Eventually we're only able to build withfeature = "_gen-bindings".I've attached a possible fix. Could you please help take a look? Thanks!