feat(build): replace unwraps by expects with useful messages in build.rs

[jaoleal]

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

[jaoleal]

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]

Okay, apparently the problem was due flag conflict and is resolved in b061f0a

CI is passing now.

[alexanderwiederin]

Thanks @jaoleal!

I think the raw cmake approach is more explicit and already works well. To fix #135, we can find a simpler solution. Could you give an example of an environment where the old approach failed?

[yancyribbens]

To fix #135, we can find a simpler solution.To fix #135, we can find a simpler solution.

I agree, maybe if you could split this out into separate commits it would be more clear. As it stands, if you are only address #135, this seems very complex.

[sedited]

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.

[jaoleal]

Ok, so:

1. 18d643a Concisely addresses Throw expect() message if deps are not found #135.
2. f8d36d Delegates command handling to the cmake crate.

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 cc crate.

By the other side, I understand if you guys still prefer the old approach. I can drop f8d36d and we can follow with 18d643a

[jaoleal]

Applied requested changes.

@yancyribbens please take a look if the current approach on a753eed suits. I tried to make the error handling more usefull than a single unwrap but not extending much.

Now theres difference between when finding cmake and executing it.

[jaoleal]

Done! @yancyribbens

[jaoleal]

Renaming the PR to match the current approach

[jaoleal]

@yancyribbens done in 4956e1b.

[jaoleal]

corrected capitalization on 26cdab4

[yancyribbens]

LGTM - thanks for improving the messaging.

[alexanderwiederin]

Thanks for the patience.

May I suggest a helper like this:

fn run_cmake(cmd: &mut Command) {
    let status = cmd
        .status()
        .expect("cmake should be installed and available in PATH");
    
    if !status.success() {
        panic!("cmake exited with status: {status}");
    }
}

From what I understand expect on status only fires when cmake fails to spawn.

[alexanderwiederin]

@jaoleal, are you still working on this? Appreciate that my review came rather late, so no pressure - but I am curious about what you think about my suggestion.

[jaoleal]

@jaoleal, are you still working on this? Appreciate that my review came rather late, so no pressure - but I am curious about what you think about my suggestion.

Hi @alexanderwiederin, since the owner of the issue thats being addressed here is yancyribbens and, with him we drove this pull request to this current state where it simply replaces the unwraps with expects, Im much aligned with his suggestions here. They gave me the impression that we may want to be simple here, maybe that helper can be a unnecessary abstraction? But to be honest I think this pr is extended for what the issue touches and in my opinion every approach that was suggested addressed the issue really well, being just a question of taste to alternate between them, im trying to leave mine as absent as possible.

@yancyribbens Can you give us a input on #136 (review) ?

[alexanderwiederin]

To clarify why I suggested the helper: from what I understand the current expect on .status() only fires if cmake fails to spawn. It won't catch a non-zero exit code, so a failed build or install would silently pass. The helper addresses that by adding the status.success().

https://doc.rust-lang.org/std/process/struct.Command.html#method.status

[yancyribbens]

I think the current state of the PR address the issue that I had. That issue was that if cmake is not installed, there is no clear message that that is the case. I should probably double check this behavior now that the PR is "finished".

@alexanderwiederin I think is pointing out that even if cmake is installed, it may fail for other reasons. Could that not be addressed with a followup issue/commit? I think it's a good idea to add the suggestion, however I think it should be specified in a separate commit message with what circumstances cmake might fail?

[alexanderwiederin]

The hesitation I have is that the second and third expects give the false impression that they catch execution failures, when .expect() on .status() only fires if cmake fails to spawn.

From what I understand the first expect, "cmake should be installed and available in PATH.", accurately describes the only failure case .expect() catches. The others don't.

I understand that this is all quite theoretical because it seems that none of us are actually testing this :)

But that is how I understand the docs.

Would you agree?

[yancyribbens]

From what I understand the first expect, "cmake should be installed and available in PATH.", accurately describes the only failure case .expect() catches. The others don't.

Good point. The lets just remove the "others" and start another issue? The only thing this PR and commit should be fixing IMO is, if cmake is not installed, that an error says so..

[alexanderwiederin]

Yes, that makes sense to me. @jaoleal, what do you think?

nit: remove the . in "cmake should be installed and available in PATH."

[jaoleal]

Okay, I think i got what you guys meant. Check if 8b79693 fits.

I have to confess that it was hard to implement that one. 😆

[jaoleal]

Hmmm the CI error appears to be a version conflict, it should be addressed in a different PR correct ?

[yancyribbens]

Can you rebase? It's hard to tell now since your branch is out of date it looks like. However, we are saying this should basically be a 1 line diff with the expect message: "cmake should be installed and available in PATH"

[jaoleal]

Can you rebase? It's hard to tell now since your branch is out of date it looks like. However, we are saying this should basically be a 1 line diff with the expect message: "cmake should be installed and available in PATH"

Yeah, thats what i tought that i pushed. I had some conflict problems with another branch, really sorry for that.

Rebased and correctly addressed the suggestions in e302a77.

The CI error remained even after rebase but i couldnt reproduce it on my end, these version conflict errors are really boring.

[yancyribbens]

This looks good to me. The CI error looks unrelated.

[alexanderwiederin]

Looks good to me! Let's hold off until the CI has been fixed.