Expose get_archiver and get_ranlib

[jonhoo]

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]

With this, I think we're also further along on #82 ?

[jonhoo]

Ah, because I now use Command::get_program to grab the program name to use in error messages, this also bumps MSRV. How unacceptable do we think that is? I could probably find a way to have it carry through the name all the way to bypass the need to use Command::get_program, though it'd be unfortunate added complexity.

[jonhoo]

Nudge on this @thomcc

[thomcc]

Sorry, thanks for the ping. Will get to it tomorrow.

[thomcc]

As part of this, I also realized that cc wouldn't pick up ARFLAGS, so I added support for that

A problem here is we already pass our own flags to it, so I think if the user provides ARFLAGS, it will collide with these. Usually I see these used in cases like ARFLAGS=Drcs, in which case https://github.com/rust-lang/cc-rs/blob/main/src/lib.rs#L2098 would collide, which is an error (at least on some ars).

(That said I am in favor of supporting ARFLAGS)

[jonhoo]

Hmm, I'm not sure what to do about that. Are you thinking we'd specifically check if s is already among the flags and if so not add it in that location?

[thomcc]

Presumably if they set ARFLAGS we don't want to pass anything?

[jonhoo]

Ohh, I see what you mean. Sure, I can make us not set flags if ARFLAGS is set!

[jonhoo]

Should that also apply if they've called Build::ar_flag?

[jonhoo]

Thinking some more about it, I'm also not sure you'd actually want to skip s for example even if ARFLAGS is set. ARFLAGS (like CFLAGS) I think is "flags that should always be set", whereas the s in the code you reference is more like a "mode" argument that you only want to pass to accomplish a particular task (as opposed to always). At least that's how it's used there — we specifically want to run ar with s to accomplish something.

I'm also a bit worried about the debuggability of "when I set ARFLAGS, cc stops working on some platforms" just because it causes s to not be passed when it was otherwise needed. But you have a better sense than I do of how likely folks are to run into this (by setting ARFLAGS to something that doesn't include s).

[jonhoo]

There are also cases like

cc-rs/src/lib.rs

Lines 2108 to 2120 in 621ba2c

	let (mut cmd, program) = self.get_ar()?;
	let mut out = OsString::from("-out:");
	out.push(dst);
	cmd.arg(out).arg("-nologo");
	for flag in self.ar_flags.iter() {
	cmd.arg(flag);
	}
	// If the library file already exists, add the library name
	// as an argument to let lib.exe know we are appending the objs.
	if dst.exists() {
	cmd.arg(dst);
	}
	cmd.args(objs);

where it feels like the arguments aren't something that'd generally be included in ARFLAGS. They're things we want to pass in addition.

[jonhoo]

nudge @thomcc

[thomcc]

Ah, sorry, thanks for the ping. Looking online for other uses of ARFLAGS (in makefiles and such) it generally looks like they include stuff like s, and completely replace the exiting set of flags.

That said, I guess we already support ar_flags and they don't have that behavior...

[jonhoo]

But presumably they don't replace all the flags. For example, input and output files still need to be specified.

I guess the question here is what behavior is less surprising and less difficult to work around. I worry about the case where suddenly things stop working because your ARFLAGS don't include s and you hit the case where we previously would inject s for you. That's harder to debug (I think) than having ar complain because you're passing s twice. We have to weigh that against the likelihood that someone wants to always pass s, even when it's not required (as per the current logic), because they won't be able to have that behavior if we explicitly add s (and ar errors on ss).

Basically: do we want to introduce hard-to-debug (but maybe rare?) errors, or make it impossible to always pass s (via ARFLAGS)?

[thomcc]

@danakj in chromium/chromium@8f52776 you seem to be griping that we don't support ARFLAGS. Can you weigh in on the semantics you'd like them to have? At the moment we're at a bit of an impasse.

[danakj]

Hi, yes! (phone typing but I’ll do my best)

In the end we changed our archive tool to one which supported -nologo on Windows (i think it was lld-llvm). However with the situation we had:

1. it was windows but we are not using msvc, we’re using clang we built ourselves
2. the default ar choice did not know “windowsisms” like -nologo
3. so archive failed

I was not looking for a way to override the mode flag of the ar tool but to (remove and) replace other flags, specifically -nologo.

tldr I think I would treat the mode flags like input files and always set them, but if ARFLAGS is set i would use that in place of any extra flags (even platform specific ones like -nologo).

[thomcc]

That seems reasonable to me. @jonhoo do you mind changing this PR to do that?

[jonhoo]

Yep, I'll make that change on Monday!

Just to confirm, that would mean we should set s even if ARFLAGS is passed in?

[danakj]

Yep

[jonhoo]

Oh, and, should the same apply if any arguments have been added using Build::ar_flag?

[thomcc]

I think so, for consistency. But I suppose there is a compatibility argument...

Edit: On Monday I'll look and see what the usage of ar_flag in the ecosystem look like. I think probably the consistency argument wins out here, and it's unlikely to break existing code.

[danakj]

Perhaps there is another way to remove flags with Build::ar_flag as the interface is more rich than an environment variable?

[jonhoo]

Done in 7a0c2af 👍

[jonhoo]

The CI failures are because of the MSRV bump as a result of using CommandBuilder::get_program.

[thomcc]

How difficult is that to avoid?

I don't know that we have a strong MSRV policy for this crate, but I'd weakly like to keep this to what works on debian stable, at least until the libc crate bumps it's MSRV (at which point almost all Rust crates will break on deb stable, so there's really no point anymore).

This is mostly to reduce false bug reports and nagging more than anything else, though, so if it's impossible to avoid it's not the end of the world.

[jonhoo]

Worked around it (I think) in de69fc7.

It's not entirely clear what the second argument to run is supposed to be — sometimes it's referred to as the command's "args", and sometimes as just a program name. I went with just the program name though.

[thomcc]

Looks great. Thanks for putting up with all the back and forth.

[thomcc]

I'll try to get a release out this weekend or so.

[jonhoo]

Thanks for putting up with all the back and forth.

Not at all — it's important to weigh the trade-offs here!

I'll try to get a release out this weekend or so.

Thank you!

Next up, alexcrichton/openssl-src-rs#173 😅