tool: Expose family() in favour of incomplete is_like_xxx() wrappers

[MarijnS95]

Instead of adding a new is_like_xxx() function every time new information is needed, or new enumeration variants would be added, provide the (now non_exhaustive) enum directly to callers to match what they're interested in.

Also removes is_like_clang_cl() again from #1357 since that did not yet make it into a release, and would only cause unnecessary duplication in our API patterns.

[NobodyXu]

Thanks!

Have a few suggestions, and I will cut release tomorrow as it's too late for me.

[NobodyXu]

Not sure if we should deprecate this now cc @madsmtm what do you think?

[MarijnS95]

Wasn't sure either, might be a bit harsh but otherwise we could forget about it.

[MarijnS95]

At the same time matching against self.family() for single variants is a bit ugly as you've seen.

[MarijnS95]

Have a few suggestions, and I will cut release tomorrow as it's too late for me.

If we incorporate this PR, shall I just go ahead and remove is_like_clang_cl() again since it's only extra duplication?

[NobodyXu]

If we incorporate this PR, shall I just go ahead and remove is_like_clang_cl() again since it's only extra duplication?

Yes i think so

[briansmith]

I suggest we hold off on doing this until the situation gets out of hand. Deprecating the current methods will cause a lot of unneeded churn, especially for projects that don't want to increase their minimum cc-rs version.

[MarijnS95]

@briansmith yeah makes sense, 1.x.x is likely to stay around for a long time. I've removed it so that this PR is ready for the proposed merge and release whenever @NobodyXu is available again (and I'm likely asleep).

[briansmith]

I wonder if it is a good idea to expose family() publicly like this at all, though? I would expect to have more thorough tests for it, first, if so.

[NobodyXu]

I wonder if it is a good idea to expose family() publicly like this at all, though?

Regarding API I think it's ok.

But since it's new API, cc @madsmtm can you take a look at this please?

I would expect to have more thorough tests for it, first, if so.

I remember we do have some tests for compiler family detection

[madsmtm]

Hmm, I don't particularly like exposing ToolFamily, for several reasons:

• I'm not certain that Msvc { clang_cl } is the right nesting, it could also be e.g. Clang { cl }.
• I'm unsure if these three families are really the right separation, e.g. we might want ToolFamily::VxWorks or ToolFamily::Emscripten one day, but if we expose ToolFamily we're forced to put those under ToolFamily::Gnu { vxworks } or ToolFamily::Clang { emscripten }.
  Basically, I'm trying to say that even with #[non_exhaustive] on the enum, it'd still be a semantic breaking change to add a new variant.
• Methods allows us to do the lookup more lazily. E.g. right now we don't use the zig_cc variable internally, so we might actually want to call is_zig_cc lazily instead when the user asks, but that would be harder if we exposed family.

[NobodyXu]

That's indeed true, we might want to reorder ToolFamily later...So perhaps we should close this PR for now and instead has more is_xxx functions?

[madsmtm]

So perhaps we should close this PR for now and instead has more is_xxx functions?

I'd prefer that, yes.

[NobodyXu]

Sorry @MarijnS95, thank you for your work but we are still too worried about the potential API breakage/subpar API, we will close this PR for now and I will cut a new release.

[MarijnS95]

Ugh, thanks for nothing. If I had known this wouldn't be merged - closed without proper discussion even - I would have at the very least reverted #1357 again. Now we're stuck with is_like_clang_cl() for eternity, which has more vague semantics than any of the counter-arguments that were given against ToolFamily 😞.

In the end in ring we don't care about this function at all anymore. Rather, we want to know if the compiler is specifically cl.exe. For now our only way to check that is is_like_msvc() && !is_like_clang_cl() but from the above it's completely uncertain if those semantics will hold forever.

Any of the is_like_*() functions don't clarify in what context they consider like-ness. Support for arguments? The way they compile (i.e. using MSVC or LLVM stack)? ToolFamily carries this load.

---

• I'm not certain that Msvc { clang_cl } is the right nesting, it could also be e.g. Clang { cl }.

Sure, we could have debated that before jumping the gun.

• I'm unsure if these three families are really the right separation, e.g. we might want ToolFamily::VxWorks or ToolFamily::Emscripten one day, but if we expose ToolFamily we're forced to put those under ToolFamily::Gnu { vxworks } or ToolFamily::Clang { emscripten }.

Can you explain why? Do they currently resolve to Gnu/Clang variants?

Basically, I'm trying to say that even with #[non_exhaustive] on the enum, it'd still be a semantic breaking change to add a new variant.

If the wrong thing currently resolves to the wrong variant, sure... I didn't expect to have to fix/clean up this popular library though.

• Methods allows us to do the lookup more lazily. E.g. right now we don't use the zig_cc variable internally, so we might actually want to call is_zig_cc lazily instead when the user asks, but that would be harder if we exposed family.

In ring there was already a concern about this, and it further signifies the point that is_like_*() is vague already and its semantics might change at any time. Likewise strange to have a zig_cc bool that's private but never used; likely a remnant from a previous workaround?

[NobodyXu]

Rather, we want to know if the compiler is specifically cl.exe. For now our only way to check that is is_like_msvc() && !is_like_clang_cl() but from the above it's completely uncertain if those semantics will hold forever.

Hmmm maybe we can add another is_cl_exe function?

[NobodyXu]

Sorry about the closing, I thought that we just don't want to expose it now and didn't think more of this as it was late for me.

Perhaps I should wait a bit longer for more feedback instead of rushing

[briansmith]

Hmmm maybe we can add another is_cl_exe function?

It would be awesome to implement #882, to provide "use MSVC-provided clang-cl instead of cl.exe" and "use MSVC-provided clang-cl instead of cl.exe" APIs, which would also do the job of discovering where clang.exe/clang-cl.exe live so they don't have to be in $PATH.

[NobodyXu]

Let me have a look, I think that's possible.

[MarijnS95]

I wasn't aware of #882, moving the burden to switch compilers (and all complication that comes with detecting it, rather than pushing it out to users via documentation that encourages them to modify PATH) into cc-rs sounds like an ideal solution to me. Keeping in mind compatibility between clang-cl/cl.exe that cannot be achieved with clang - think of it like switching between the MSVC and LLVM toolchain while using the correct/compatible tool (to not have to modify users' CFLAGS etc).

Let's investigate that before adding is_cl_exe() etc.