feat(build): move to cmake crate for building script in build.rs#136
feat(build): move to cmake crate for building script in build.rs#136jaoleal wants to merge 2 commits intosedited:masterfrom
Conversation
|
I got a problem while trying to build on windows, still working on that. But feedback on overall approach does apply, specially regarding the cmake crate. |
jaoleal
force-pushed
the
cmake_crate
branch
2 times, most recently
from
36ef06b to
b061f0a
Compare
|
Okay, apparently the problem was due flag conflict and is resolved in b061f0a CI is passing now. |
|
I agree with @alexanderwiederin and @yancyribbens . I think we should strive to have as few build and runtime dependencies as possible. I think there should be an easier way to surface a nicer error to address #135. |
Replace bare .unwrap() calls on cmake Command invocations with descriptive error handling. The first cmake call (configure) now specifically detects ErrorKind::NotFound and reports that cmake is required with installation instructions. The build and install calls use .expect() with clear messages and check exit status.
`libbitcoinkernel-sys/build.rs` was executing a cmake binary from the environment and doing some manual flag and path configuration. The presented changes delegate the same job to the cmake crate which also handles dependencies accross environments.
|
Ok, so:
My rationale for addressing #135 with the cmake crate is thats the ""oficial"" way to do it, found it while searching how should i deal with Build Scripts, since it already have some more granular error handling and error messages i thought for it to be a better fit to address #135. The cmake is well-maintained and its only dependency is cc, which is already added in libbitcoinkernel-sys/Cargo.toml. On my slow laptop (i3-13gen + 8gbram) the build time were negligible. And this approach matches the same approach that was taken when using the By the other side, I understand if you guys still prefer the old approach. I can drop f8d36d and we can follow with 18d643a |
libbitcoinkernel-sys/build.rs
Outdated
| @@ -56,15 +56,15 @@ fn main() { | |||
| .arg(build_config) | |||
| .arg(format!("--parallel={num_jobs}")) | |||
| .status() | |||
| .unwrap(); | |||
| .expect("Failed to run cmake build"); | |||
There was a problem hiding this comment.
This doesn't really add any useful information, so I wouldn't have bothered changing this. Although, you could improve this if you want to be what you expect, for example that the config and build directory are valid.
libbitcoinkernel-sys/build.rs
Outdated
|
|
||
| Command::new("cmake") | ||
| .arg("--install") | ||
| .arg(&build_dir) | ||
| .arg("--config") | ||
| .arg(build_config) | ||
| .status() | ||
| .unwrap(); | ||
| .expect("Failed to run cmake install"); |
libbitcoinkernel-sys/build.rs
Outdated
| @@ -42,7 +42,7 @@ fn main() { | |||
| .arg("-DENABLE_IPC=OFF") | |||
| .arg(format!("-DCMAKE_INSTALL_PREFIX={}", install_dir.display())) | |||
| .status() | |||
| .unwrap(); | |||
| .expect("cmake is required to build libbitcoinkernel. Please install cmake and ensure it is in your PATH."); | |||
There was a problem hiding this comment.
This seems better, and is how I would have done it. Although, I probably would have just wrote cmake should be installed.
4 participants
libbitcoinkernel-sys/build.rs` was executing a cmake binary from the environment and doing some manual flag and path configuration. The presented changes delegate the same job to the cmake crate which should handle dependencies accross environments.
But im not certain if the choice of not using this crate is on purpose.
fix: #135