-
Notifications
You must be signed in to change notification settings - Fork 258
build: fix rebuild rules for no prebuilt bindings #450
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
Conversation
314b279
to
9b82bf7
Compare
grpc-sys/build.rs
Outdated
match target.as_str() { | ||
"x86_64-unknown-linux-gnu" | "aarch64-unknown-linux-gnu" => { | ||
file_path = PathBuf::from(env::var("CARGO_MANIFEST_DIR").unwrap()) | ||
.join("bindings") | ||
.join(format!("{}-bindings.rs", &target)); | ||
println!( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about add a comment so that future refactor will not break it again?
grpc-sys/build.rs
Outdated
match target.as_str() { | ||
"x86_64-unknown-linux-gnu" | "aarch64-unknown-linux-gnu" => { | ||
file_path = PathBuf::from(env::var("CARGO_MANIFEST_DIR").unwrap()) | ||
.join("bindings") | ||
.join(format!("{}-bindings.rs", &target)); | ||
println!( | ||
"cargo:rerun-if-changed=bindings/{}", | ||
file_path.as_path().as_os_str().to_str().unwrap() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest to use relative path. Otherwise sharing building cache can become a problem.
@BusyJay made the changes, please take another look when you can. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Please signoff your commits. |
Note all commits in the PR needs to be signoff. |
…it is being used (fix tikv#449) Signed-off-by: Eran Rundstein <[email protected]>
03a35e9
to
1e9392f
Compare
Rebased and force-pushed to fix that @BusyJay |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hunterlxt PTAL
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Thank you for moving quickly on merging this! |
This fixes the regression introduced by 0bb4c60#diff-fc1b9721ad58e6a299d6d6633dbc46caR335 and documented on #449