Skip to content
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

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

Closed
wants to merge 2 commits into from

Conversation

jonhoo
Copy link
Contributor

@jonhoo jonhoo commented Oct 26, 2022

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.

A second try at alexcrichton#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.
Sometimes `gcc-ar` is broken, so when both are available we should
prefer non-gcc-ar. See also rust-lang/cc-rs#741.
@jonhoo
Copy link
Contributor Author

jonhoo commented Oct 31, 2022

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
Copy link
Owner

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

@jonhoo
Copy link
Contributor Author

jonhoo commented Nov 4, 2022

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
Copy link
Owner

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
Copy link
Contributor Author

jonhoo commented Nov 8, 2022

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
Copy link
Contributor Author

jonhoo commented Nov 29, 2022

Gentle nudge on this

@jonhoo
Copy link
Contributor Author

jonhoo commented Dec 12, 2022

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 jonhoo closed this Dec 12, 2022
jonhoo pushed a commit to jonhoo/cc-rs that referenced this pull request Dec 12, 2022
This PR exposes cc's inferred archiver through a similar mechanism as
`get_compiler`: with `get_archiver`/`try_get_archiver`.

As part of this, I also realized that cc wouldn't pick up `ARFLAGS`, so
I added support for that. Through that, I also realized that there was
currently one place where we didn't respect `Build::ar_flags` when ar
was called, so I fixed that too.

Since ranlib is exposed more or less exactly like ar, I also added a
`get_ranlib`/`try_get_ranlib` combination. This is, in part, to enable
`openssl-src` to move to re-use `cc`'s AR (and now RANLIB) detection
rather than rolling its own. See alexcrichton/openssl-src-rs#164.

Note that I didn't re-use `Tool` for the return value for these getter
functions, and instead just exposed `Command` directly. `Tool` has a
number of relatively compiler-specific things, so it felt wrong to
use. One important caveat to this is that `Command::get_program` was
only added in Rust 1.57.0, so users on older versions of Rust won't have
a way to recover the inferred `ar` program path, but that feels like
it's probably okay given that we didn't even expose `get_archive` until
now.
@jonhoo
Copy link
Contributor Author

jonhoo commented Dec 12, 2022

rust-lang/cc-rs#763 is step 1.

jonhoo pushed a commit to jonhoo/openssl-src-rs that referenced this pull request Dec 12, 2022
Note that `Command::get_program` and `Command::get_args` both stabilized
in Rust 1.57.0, and so implicitly bump this crate's MSRV.

Depends on rust-lang/cc-rs#763.

Replaces alexcrichton#164.
@jonhoo
Copy link
Contributor Author

jonhoo commented Dec 13, 2022

And #173 is the eventual PR that brings that into openssl-src.

jonhoo pushed a commit to jonhoo/openssl-src-rs that referenced this pull request Jan 30, 2023
Note that `Command::get_program` and `Command::get_args` both stabilized
in Rust 1.57.0, and so implicitly bump this crate's MSRV.

Depends on rust-lang/cc-rs#763.

Replaces alexcrichton#164.
alexcrichton pushed a commit that referenced this pull request Feb 1, 2023
Note that `Command::get_program` and `Command::get_args` both stabilized
in Rust 1.57.0, and so implicitly bump this crate's MSRV.

Depends on rust-lang/cc-rs#763.

Replaces #164.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants