search

alexcrichton / openssl-src-rs

Source code and logic to build OpenSSL from source
Apache License 2.0
69 stars 114 forks source link

Correctly infer ranlib/ar for cross-gcc toolchains #164

Closed jonhoo closed 1 year ago

jonhoo commented 2 years ago

A second try at #163.

The GCC convention is that if the toolchain is named $target-gnu-gcc, then ranlib and ar will be available as $target-gnu-gcc-ranlib and $target-gnu-gcc-ar respectively. The code as written would infer them to be $target-gnu-{ranlib,ar}, which will only work if the tools from binutils (which follow that convention) are on $PATH.

I've also updated the logic in line with the cc crate to check that the inferred AR/RANLIB is actually executable before setting it as an override.

See also rust-lang/cc-rs#736.

jonhoo commented 2 years ago

Gentle nudge on this @alexcrichton — this change has now been released in cc, so may be good to update this as well so the two crates agree.

alexcrichton commented 2 years ago

Sorry but honestly that seems hastily merged with https://github.com/rust-lang/cc-rs/pull/741 following shortly afterwards. I'd prefer to let that bake with user experience before causing more problems here.

jonhoo commented 2 years ago

Though after the latest change, this now preserves the existing semantics of preferring the non prefixed binaries. It effectively just adds the gcc-prefixed binaries as another (secondary) alternative to try. The only exception being if the non prefixed binaries don't exist/aren't executable, which is a broken state anyway.

alexcrichton commented 2 years ago

Could this perhaps be exposed through the cc crate if you're interested in adding this? I'd rather not use yet-more process spawning to figure out if something is there and otherwise duplicating this logic already in the cc crate. Ideally this would do a PATH search but that feels like it's quickly adding complexity for a relatively niche case.

jonhoo commented 2 years ago

You mean have openssl-src use cc to determine the ar to use? Yeah, we could probably do that, though I assumed there was a reason it wasn't doing it already? cc also doesn't (to my knowledge) detect ranlib, so we'd either need to add that to cc or retain custom logic here for finding ranlib.

jonhoo commented 1 year ago

Gentle nudge on this

jonhoo commented 1 year ago

Synced with Alex offline, and I'm going to close this and open a new PR that moves openssl-src to just use cc's detection directly. Given that cc doesn't yet have ranlib detection, I'll have to land that first though.

jonhoo commented 1 year ago

https://github.com/rust-lang/cc-rs/pull/763 is step 1.

jonhoo commented 1 year ago

And https://github.com/alexcrichton/openssl-src-rs/pull/173 is the eventual PR that brings that into openssl-src.