Default to llvm-ar when compiling for wasm.

[dot-asm]

Defaulting to llvm-ar facilitates cases when system ar(1) doesn't
support any given cross-compile target. This is because accompanying
llvm-ar is known to match targets supported by the clang installation
at hand.

[dot-asm]

CI failures appear to have everything to do with the fact that cargo test fails in on Windows with Rust 1.58, but not with Rust 1.57. I mean it's something in 1.58 that makes it fail. This naturally falls beyond the scope of this PR. [Even though it doesn't mean that cc-rs doesn't need an adjustment.]

[dot-asm]

On a related note one can wonder if it would be appropriate to extend this approach beyond a clang-specific default. Most notably gcc also recognizes -print-search-dirs option and cross-gcc-compiler can steer you toward target-specific ar. I mean if I traverse programs from arm-linux-gnueabi-gcc I do find arm-specific ar. To be more specific, it looks like it's possible to replace the current if-else with match compiler.family and use -print-search-dirs to locate ar for ToolFamily::Gnu, llvm-ar for ToolFamily::Clang, and just picklib.exe for ToolFamily::Msvc... Or at least prioritize such match and preserve the old code as a legacy fallback...

[alexcrichton]

Sorry but I don't think I have the energy to shepherd and merge this change. The cc crate is used in quite a few different environments that I know I can't even begin to enumerate and changing the core default behavior of searching for ar is something that will likely inevitably be fraught with errors and bugs to track down in esoteric environments and that's not something I'm willing to take on at this time.

I don't know of anything specifically wrong with this patch, but maintaining this crate for long enough has shown to me that even if issues can't immediately be seen they're likely still lurking unfortunately. If this can be done in a way that's opt-in or something like that I would be more ok merging because the risk would be much lower.

[dot-asm]

That's unfortunate:-(

For reference, the suggestion is a generalization of a problem with wasm target. While ar(1) can collect literally any type of files in the .a-archive, it has a responsibility to maintain a symbol table for the binary objects in the .a-archive. The table in question can be examined with nm -s. Customarily /usr/bin/ar is aware only of native execution format and attempt to use it with foreign one makes it treat the binary objects effectively as non-binary and not generate the table. And rationale is that if clang --target=xyz works, accompanying llvm-ar would be effectively obliged to manage the xyz target correctly.

Before I close this, a question. Would it be helpful to open an issue and challenge clang users to provide feedback if builds with AR environment variable set to llvm-ar work reliably? Or maybe if they even have to set it in order to get it working? Especially in cross-compile scenarios...

Cheers.

[alexcrichton]

Unfortunately, at least in my experience, I don't know how to get feedback on a change like this. There's so many ways that C build systems are configured in my experience all you can really do is push the change, see what breaks, and evaulate it if it's worth it to back out or not. That's not a great way of managing this however.

If you wanted to make this specific to the wasm target though that seems reasonable to me. Making this more targeted that can theoretically get generalized in the future but not necessarily immediately seems plausible. I don't think this crate is as heavily used with wasm and AFAIK all wasm C compilation goes through clang right now with a pretty well-defined set of compilers.

[dot-asm]

If you wanted to make this specific to the wasm target though that seems reasonable to me.

Adjusted accordingly.

Making this more targeted that can theoretically get generalized in the future but not necessarily immediately seems plausible.

Like for the next major release when possible breakage can be tolerated. There are more cleanups that one can implement...

[alexcrichton]

Sorry but I've decided that the current development trajectory of cc is not one that I can maintain, so this crate will need to grow new maintainers before merging.

[dot-asm]

In addition I default to llvm-lib in clang-cl compilations. This is an alternative to #668.

[dot-asm]

Just in case, as for o-wasm label. It's actually two-part now, wasm and Windows. And as discussed in the commentary, it's possible to make a case for extending the approach, defaulting to llvm-ar, to all clang compilations. And even to gcc (arguably less error-prone than recent gcc-ar changes:-). There is only one potential problem with the latter. gcc for Windows doesn't seem to enforce UTF-8 in -print-search-dirs output, which makes it uncertain if there are non-ASCII characters in the paths...

[ChrisDenton]

Could you separate the Windows llvm-lib changes in to another PR? I'd feel confident in merging that asap and I don't think it needs to be tied to the wasm changes.

[dot-asm]

I've opened #758 and marked this one draft, as it would require a re-adjustment once #758 is merged.

[dot-asm]

Back from draft, now as wasm-only thing.

Just in case for reference. As it stands now, the linking procedure (on non-emscripten wasm targets) fails with "archive has no index" in .a file generated by cc-rs. This modification resolves the problem.

[dot-asm]

Re-based just in case.

[dot-asm]

Re-based.

To recap. The problem is that the linking procedure (on non-emscripten wasm targets) fails with "archive has no index" in .a file generated by cc-rs. For completeness sake one can note that Rust 1.67 doesn't seem to require the index, presumably at the cost of longer linking times.

Just in case. One can dispute the placement of the suggested else if segment. In [the] sense that one could have placed it [in] the match tool_opt below. Such placement is would arguably be less readable. Indeed, the name of the emscripten target is wasm32-unknown-emscripten, so that moving the else if target.starts_with("wasm32") below raises the question if it will override the if target.contains("emscripten"). Well, it won't because of the initial Some(t) => t, but it's not as apparent as reading the sequence if target.contains("emscripten") ... else if target.starts_with("wasm32"). But if the alternative placement is considered preferred nevertheless, just say so. And of course if one chooses to extend the suggested method (of examining -print-search-dirs output) to all clang targets, then it would have to be moved below, presumably as as first if case...

[dot-asm]

But if the alternative placement is considered preferred nevertheless, just say so.

Though moving would entail restructuring of the match tool_opt, because it would be appropriate to have an option to return None and fall back on some default...

[dot-asm]

if one chooses to extend the suggested method (of examining -print-search-dirs output) to all clang targets, then it would have to be moved below, presumably as the first if case...

On a tangential note, yet in favour of this suggestion;-) The first if in the match tool_opt, the if target.contains("android") one, is effectively a hinder. As there are no format!("{}-{}", target.replace("armv7", "arm"), tool)-s provided since a while back, since the switch to clang or shortly after...

[Mrmaxmeier]

This fixes the archive has no index; run ranlib to add one issue from #531 and feels minimally invasive.
(I'm posting this as a well-intentioned bump and for visibility, since searching for the run ranlib error only returns the closed PR for some reason 🙂)

[thomcc]

This seems fine to me, especially since it's localized to wasm.