Detect whether compiler is Clang at nix eval time

[YorikSar]

While working on #215 I noticed that we can use cc.isClang to detect if compiler is Clang. It requires us to pull this flag through cc package wrappers. I split this from #215 because this affects our API: compiler packages in Nix would now be required to provide isClang flag that might not be the case for compilers provided by custom Nix expressions.

[aherrmann]

Nice, thanks!

[dmadisetti]

This breaks for me with a custom toolchain? I think we should have a fallback mechanism, or a required module for custom toolchains (to allow for attribute defaults)

Cannot build Nix attribute ''.
Command: [/run/current-system/sw/bin/nix-build, /home/dylan/.cache/bazel/_bazel_dylan/8bbdcf70ee87482f767b06aac75f66d1/external/nixpkgs_config_cc_info/external/rules_nixpkgs_cc/cc.nix, "-A", "", "--out-link", "bazel-support/nix-out-link", "--argstr", "ccType", "ccTypeExpression", "--arg", "ccExpr", "import ./external/toolchain.nix", "-I", "nixpkgs=/home/dylan/.cache/bazel/_bazel_dylan/8bbdcf70ee87482f767b06aac75f66d1/external/nixpkgs/nixpkgs"]
Return code: 1
Error output:
error: attribute 'isClang' missing

       at /home/dylan/.cache/bazel/_bazel_dylan/8bbdcf70ee87482f767b06aac75f66d1/external/nixpkgs_config_cc_info/external/rules_nixpkgs_cc/cc.nix:282:12:

          281|     IS_CLANG=(
          282|       ${if cc.isClang then "True" else "False"}
             |            ^
          283|     )
(use '--show-trace' to show detailed location information)

Maybe ${if cc ? isClang then if cc.isClang then "True" else "False" else "$(fallback command)"}

[aherrmann]

@dmadisetti Thanks for raising this.

This breaks for me with a custom toolchain?

Would it be feasible to extend the custom toolchain such that cc.isClang is defined?

Otherwise, yes, a fallback branch seems like a reasonable option.

[dmadisetti]

@aherrmann it is possible to define cc.isClang on the toolchain, but the error is not immediately obvious. I think a little defensive programming is the right option here.

I think the nicest option would be to create toolchain modules (I think one can call pkg.lib.evalModules with an options definition and the custom expr?). This is self documenting and can easily provide defaults + more options (like cxx_opts #218). The downside is that it may take a bit of work, and for consistency, should be replicated for the other toolchains.

Failing that, the branch should be fine! I've been running a patch of rules for awhile, I just wanted to raise my pain points upstream.

Let me know if we should move this concern to its own issue

[aherrmann]

@aherrmann it is possible to define cc.isClang on the toolchain, but the error is not immediately obvious. I think a little defensive programming is the right option here.

Perhaps improving the error message is the right path then. I think this could also be achieved with a branch like you suggest: If the field is missing, the error could instruct the user to provide it.

Thinking a bit more about it, I wonder if this issue is related to what @avdv encountered in tweag/rules_haskell#1750 (comment).

[YorikSar]

I would recommend to not fall back to assuming that it's not Clang. When I ran into this issue, it was not trivial to debug the error. I think recommending people to add passthru = { inherit (cc) isClang; } or something similar if it's missing is the way to go. By the way, wrapCCWith is already doing this.

[avdv]

I would recommend to not fall back to assuming that it's not Clang.

AFAIU, the idea was not to use a default value, but to fall back to the old behavior of calling cc -v | grep clang in case isClang is not available on the toolchain.

We could even print a deprecation warning in this case.

Also, I like the idea about the module! Currently you need to be aware of the current expectations and that is not very user friendly.

[aherrmann]

AFAIU, the idea was not to use a default value, but to fall back to the old behavior of calling cc -v | grep clang in case isClang is not available on the toolchain.

Yes, that was also my understanding. This sounds far less dangerous than a hard-coded fall-back. @YorikSar were there any issues with that approach that #216 fixed?

Also, I like the idea about the module! Currently you need to be aware of the current expectations and that is not very user friendly.

To be sure I'm on the same page. This is talking about NixOS modules, correct? I'm not quite sure I understand how they fit here. Could you perhaps sketch out what this would look like?

[YorikSar]

AFAIU, the idea was not to use a default value, but to fall back to the old behavior of calling cc -v | grep clang in case isClang is not available on the toolchain.

Yes, that was also my understanding. This sounds far less dangerous than a hard-coded fall-back. @YorikSar were there any issues with that approach that #216 fixed?

As far as I remember, cc -v approach just didn't work, and we ended up with IS_CLANG=False which lead to wrong arguments being passed to the compiler. I think, cc -v was just failing or something like that.

[aherrmann]

I think, cc -v was just failing or something like that.

Oh, I see. I think in this case this should be fixed by the switch to using nix-support/cc-flags. However, it shows that cc -v | grep clang is a little too naive. It should be able to distinguish three states: Is clang, is not clang, is broken.