CompileBinaries Struct

[hone]

This is a change to #199. Let's leverage some Rust types to reduce some code duplication and group the cc/ld concepts together into a single struct. This moves all the defaults into const at the top of the file. In addition, I took @edmorley's suggestion to add some help/binary checking for the musl-tools on linux as well.

[edmorley]

Thank you for demonstrating this alternative via code - much easier to visualise! :-)

With the reduction of binaries from 4 to 2 (after dropping the *-ld variants), I feel like this is perhaps a bit over-complex for replacing two which calls?

If we wanted to simplify cross_compile_env(), then perhaps adding a single which() function that performed the .map_err and .into_os_string() would be a good middle ground?

[edmorley]

We don't need the *-ld variants (whilst they are present in the cargo-make examples and #199, they are not needed and differ from the more commonly used configuration):
https://github.com/Malax/libcnb.rs/pull/199#discussion_r757620799

[edmorley]

The env vars do not need to be set on Linux, since cargo/rustc correctly finds the compiler there.
Ideally in the future it will be able to do so for the OS X musl-cross too:
rust-lang/cargo#4133

As such I think we shouldn't set env vars on Linux, since I see them as more of a workaround, rather than necessary configuration.

[hone]

@edmorley thoughts on just keeping the which checks?

[hone]

With the reduction of binaries from 4 to 2 (after dropping the *-ld variants), I feel like this is perhaps a bit over-complex for replacing two which calls?

If we wanted to simplify cross_compile_env(), then perhaps adding a single which() function that performed the .map_err and .into_os_string() would be a good middle ground?

For me the most important thing I wanted out of the PR are 2 things:

1. const at the top
2. reducing code duplication (since I think we also want to check the binaries for linux and maybe windows some day)

What about just keeping the Binary struct which has the which implementation and we save these as constants. I can throw a commit up to show on this PR.

[hone]

I also think we should only insert env vars if the env var isn't set already.

[Malax]

I'd much prefer having this discussion either directly in #199 or a subsequent PR with potential improvements. It's hard to keep track of both the original PR with 150+ comments, the resulting changes and then this PR in on top.

@hone would you be fine rebasing this to main after #199 is merged?

[schneems]

I like using constants rather than magic strings. I also like putting the which behavior on a type. I also agree it's best done afterward as #199 is a moving target.

[hone]

I'd much prefer having this discussion either directly in #199 or a subsequent PR with potential improvements. It's hard to keep track of both the original PR with 150+ comments, the resulting changes and then this PR in on top.

@hone would you be fine rebasing this to main after #199 is merged?

yeah, I'm fine either way. None of these are blocking changes to #199. My original hope is this was going to be a quick merge in or reject.

[edmorley]

@hone Can this be closed out? Comparing the implementation here with the one that landed in #199, I see many of the suggestions were incorporated, plus the module was refactored quite a bit on top:
https://github.com/Malax/libcnb.rs/blob/main/libcnb-cargo/src/cross_compile.rs

As such, I don't think there is anything else more we need from this PR?

[hone]

Yeah, let's close this out for now. I can open up a new PR if I want to bring some of these ideas forward.