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.

Sign up for GitHub

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

Jump to bottom

grpc-sys: Fix build failure on uncommon linux build targets #644

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

grpc-sys: Fix build failure on uncommon linux build targets #644

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 19 additions & 18 deletions grpc-sys/build.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -481,24 +481,24 @@ fn bindgen_grpc(file_path: &Path) {
// need to be updated by default unless the _gen-bindings feature is specified.
// Other platforms use bindgen to generate the bindings every time.
fn config_binding_path() {
let target = env::var("TARGET").unwrap();
let file_path: PathBuf = 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" => {
// Cargo treats nonexistent files changed, so we only emit the rerun-if-changed
// directive when we expect the target-specific pre-generated binding file to be
// present.
println!("cargo:rerun-if-changed=bindings/bindings.rs");

PathBuf::from(env::var("CARGO_MANIFEST_DIR").unwrap())
.join("bindings")
.join("bindings.rs")
}
_ => PathBuf::from(env::var("OUT_DIR").unwrap()).join("grpc-bindings.rs"),
let file_path: PathBuf;

#[cfg(not(any(
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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").

Copy link
Member

Choose a reason for hiding this comment

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

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.

feature = "_gen-bindings",
not(all(
any(target_os = "linux", target_os = "macos"),
any(target_arch = "x86_64", target_arch = "aarch64")
))
)))]
{
// Cargo treats nonexistent files changed, so we only emit the rerun-if-changed
// directive when we expect the target-specific pre-generated binding file to be
// present.
println!("cargo:rerun-if-changed=bindings/bindings.rs");

file_path = PathBuf::from(env::var("CARGO_MANIFEST_DIR").unwrap())
.join("bindings")
.join("bindings.rs")
};

#[cfg(any(
Expand All @@ -509,6 +509,7 @@ fn config_binding_path() {
))
))]
{
file_path = PathBuf::from(env::var("OUT_DIR").unwrap()).join("grpc-bindings.rs");
// On some system (like Windows), stack size of main thread may
// be too small.
let f = file_path.clone();
Expand Down