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

Use cc for ar/ranlib detection #173

Merged
merged 1 commit into from
Feb 1, 2023

Conversation

jonhoo
Copy link
Contributor

@jonhoo jonhoo commented 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 #164.

@jonhoo
Copy link
Contributor Author

jonhoo commented Dec 12, 2022

Marked this as a draft until the cc-rs PR lands.

@jonhoo
Copy link
Contributor Author

jonhoo commented Dec 13, 2022

cargo package fails because it doesn't respect the [patch.crates-io].

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 jonhoo marked this pull request as ready for review January 30, 2023 22:09
@jonhoo
Copy link
Contributor Author

jonhoo commented Jan 30, 2023

@alexcrichton The features we needed from cc have now landed, so I've marked the PR as ready for review. The biggest discussion point here I think is that this bumps MSRV for the crate since it uses CommandBuilder::get_args which requires 1.57.0. We could extend cc so that it also provided a mechanism for getting the command and arguments separately, but it feels unnecessary given that we now have the "right" way to do this via std.

@alexcrichton alexcrichton merged commit 6d6109f into alexcrichton:main Feb 1, 2023
@jonhoo jonhoo deleted the cc-detection branch February 1, 2023 16:25
@jonhoo
Copy link
Contributor Author

jonhoo commented Feb 2, 2023

Do you have a rough sense for when this may land in a release? I have a couple of builds in semi-weird environments that are currently failing and would be fixed by this :)

@alexcrichton
Copy link
Owner

I believe an OpenSSL security release is happening next week, so next week.

@jonhoo
Copy link
Contributor Author

jonhoo commented Feb 2, 2023

Interesting — that makes me wonder if we might want to release this now to get a heads up in case it breaks anyone's builds. I worry about getting into a position where a security fix goes out at the same time as a change that may break people's builds, as it may mean some people end up not being able to take the security fix.

@jonhoo
Copy link
Contributor Author

jonhoo commented Feb 21, 2023

Ah, I just realized this only affects the 300 major version. I assume it'd also be okay to backport to 111? I'm happy to do that assuming it's acceptable.

@alexcrichton
Copy link
Owner

Yes that's fine, I can make a release after a PR to the 111 branch

jonhoo pushed a commit to jonhoo/openssl-src-rs that referenced this pull request Feb 27, 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.

Backport of alexcrichton#173.
@jonhoo
Copy link
Contributor Author

jonhoo commented Feb 27, 2023

Backport to 111 in #180

alexcrichton pushed a commit that referenced this pull request Feb 27, 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.

Backport of #173.
@Skepfyr
Copy link

Skepfyr commented Mar 12, 2023

Note this does appear to have broken rustup builds:
rust-lang/rustup#3263
sfackler/rust-openssl#1839

@alexcrichton
Copy link
Owner

I don't know how best to fix this myself, unfortunately.

@jonhoo
Copy link
Contributor Author

jonhoo commented Mar 13, 2023

As per this comment I don't think this is a bug in openssl or openssl-src — it's really just that cc's logic for detecting ar doesn't (always) work for illumos, and its logic needs updating. Once that happens, openssl-* will also build fine again.

}
let ranlib = cc.get_ranlib();
configure.env("RANLIB", ranlib.get_program());
if ranlib.get_args().count() == 0 {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If count is 0, then we set the RANLIBFLAGS to empty string?

Why we need to do that?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So that we explicitly do not inherit RANLIBFLAGS from upstream if it's been overridden in the environment. At least I think that was the thinking.

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.

4 participants