clang-tools: Improve wrapper script, add tests

[Patryk27]

clang-tools invokes clangd through a wrapper script whose sole purpose is to extend CPATH and CPLUS_INCLUDE_PATH envs, so that clangd is later able to find built-in and stdlib headers.

This script does two things wrong:

1. We extend CPATH et al with entries from both @clang@/nix-support/libc-cflags and @clang@/resource-root/include, which is kind of a self-made problem, since clangd already has some header auto-detection logic which we (used to) circumvent (for more details, see the diff).

2. We extend CPATH et al unconditionally, breaking the --query-driver option. I'm not sure what's the perfect solution here, but skipping this logic when we detect --query-driver should be good enough. I mean, ideally we wouldn't be playing with CPATH etc. whatsoever, but well.

Closes #351962
Closes #348791
Closes #345704

Things done

• Built on platform(s)
  • x86_64-linux
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin
• For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
  • sandbox = relaxed
  • sandbox = true
• Tested, as applicable:
  • NixOS test(s) (look inside nixos/tests)
  • and/or package tests
  • or, for functions and "core" functionality, tests in lib/tests or pkgs/test
  • made sure NixOS tests are linked to the relevant packages
• Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
• Tested basic functionality of all binary files (usually in ./result/bin/)
• 24.11 Release Notes (or backporting 23.11 and 24.05 Release notes)
  • (Package updates) Added a release notes entry if the change is major or breaking
  • (Module updates) Added a release notes entry if the change is significant
  • (Module addition) Added a release notes entry if adding a new NixOS module
• Fits CONTRIBUTING.md.

---

Add a 👍 reaction to pull requests you find important.

[Patryk27]

Note that I'm not sure how to test --query-driver, my fix is just a hunch - cc @iniw, @lukash, could you play with it?

[iniw]

I'm not sure how great of a fix this is. --query-driver doesn't actually do anything all that special, it's only a glob that acts as a whitelist for which programs clangd is allowed to execute to find the appropriate headers. For example, mine is set to **, meaning all paths are allowed, because I only work on projects that I trust. Anyways, my point is that changing behavior depending on the existence of that flag is not ideal, IMO.

My question is: why do we even need the wrapper's shenanigans in the first place? clangd is very good at finding out about system headers and compiler's resource directories given just a compile_commands.json - why do we need anything else? Is the goal for the wrapped package to still find nixos's system headers when a compile_commands.json isn't found? If that's the case I'd prefer just dropping support for that entirely and instead document the fact that nixpkg's clangd will only properly work when given a valid compile_commands.json, which is already required anyways when working with third-party libraries and/or custom compile options such as C++ version, etc.

EDIT: Forgot to mention, the fix did work.

[iniw]

And as mentioned in the other issue raised on this matter, a clangd dev does seem to agree that adding these implicit paths is at the very least unusual.
clangd/clangd#2181 (comment)

[lukash]

Thanks for your work on this, @Patryk27.

To me the --query-driver detection also seems sketchy, as @iniw said, it's just a mask. The check will fail at least in the case where the --query-driver won't match the actual compiler that's used. Might work to an extent when the user knows what they're doing, but still will be quirky. There's likely no robust mechanism to achieve what NixOS needs in clangd.

Being ignorant of the circumstances, what's in @clang@/nix-support/libc-cflags (and the C++ equivalent thereof) and why is it needed?

Is there an easy way to test the PR? I have a copy of the clang-tools default.nix I made some time ago and hacked the wrapper, that works. But since then the way that default.nix is called has changed and simple drop-in of the code isn't working for me (I'm calling it like this in a flake: my-clang-tools = nixpkgs.legacyPackages.${system}.callPackage ../clang-tools/clang-tools.nix {llvmPackages = pkgs.llvmPackages_18;};). I'll try to figure it out when I have some time, but not sure when that'll be.

[lukash]

Well I copy-pasted the code from the PR and hacked at it until it "worked".

clangd seems to work well and the change to install its headers to the place it's looking for them is definitely correct. The workaround with --query-driver seems to improve the situation in general. It is strictly better for most cases and for the case of using --query-driver but not having the actual compiler match, the user has the possibility to fix it by correcting the --query-driver. Current state has broken cases with no workarounds for the user.

So IMO it'd be good to merge it, but a note explaining the workaround with the --query-driver detection should be definitely included in the wrapper script.

With the (hacked up) PR code, though, I'm not able to run clang-format from the package:

$ clang-format
/nix/store/cb77k2bnkb1fvr25pb9chv3baj1x6x1x-clang-tools-18.1.8/bin/clang-format: line 44: /nix/store/nzyxg89vqxhdw884n50c2p5k1vrgibka-clang-unwrapped-with-lib/bin/clang-format: No such file or directory

$ which clang-format
/nix/store/cb77k2bnkb1fvr25pb9chv3baj1x6x1x-clang-tools-18.1.8/bin/clang-format

$ ls -l /nix/store/cb77k2bnkb1fvr25pb9chv3baj1x6x1x-clang-tools-18.1.8/bin/clang-format
lrwxrwxrwx 1 root root 6 led  1  1970 /nix/store/cb77k2bnkb1fvr25pb9chv3baj1x6x1x-clang-tools-18.1.8/bin/clang-format -> clangd

$ ls -l /nix/store/cb77k2bnkb1fvr25pb9chv3baj1x6x1x-clang-tools-18.1.8/bin/clangd
-r-xr-xr-x 1 root root 1499 led  1  1970 /nix/store/cb77k2bnkb1fvr25pb9chv3baj1x6x1x-clang-tools-18.1.8/bin/clangd

$ cat /nix/store/cb77k2bnkb1fvr25pb9chv3baj1x6x1x-clang-tools-18.1.8/bin/clangd
#!/nix/store/1xhds5s320nfp2022yjah1h7dpv8qqns-bash-5.2p32/bin/sh

buildcpath() {
  local path after

  while (( $# )); do
    case $1 in
        -isystem)
            shift
            path=$path${path:+':'}$1
            ;;

        -idirafter)
            shift
            after=$after${after:+':'}$1
            ;;
    esac

    shift
  done

  echo $path${after:+':'}$after
}

# When user passes `--query-driver`, avoid extending `CPATH` et al, since we
# don't want to "infect" user-specified toolchain and headers with our stuff.
extendcpath=true

for arg in "$@"; do
  if [[ "${arg}" == \-\-query\-driver* ]]; then
    extendcpath=false
  fi
done

if [ "$extendcpath" = true ]; then
  export CPATH=${CPATH}${CPATH:+':'}$(buildcpath ${NIX_CFLAGS_COMPILE} \
                                                 $(</nix/store/nr95z75f3hj5z7a0bddqpx9wjm47mvbd-clang-wrapper-17.0.6/nix-support/libc-cflags))

  export CPLUS_INCLUDE_PATH=${CPLUS_INCLUDE_PATH}${CPLUS_INCLUDE_PATH:+':'}$(buildcpath ${NIX_CFLAGS_COMPILE} \
                                                                                        $(</nix/store/nr95z75f3hj5z7a0bddqpx9wjm47mvbd-clang-wrapper-17.0.6/nix-support/libcxx-cxxflags) \
                                                                                        $(</nix/store/nr95z75f3hj5z7a0bddqpx9wjm47mvbd-clang-wrapper-17.0.6/nix-support/libc-cflags))
fi

exec -a "$0" /nix/store/nzyxg89vqxhdw884n50c2p5k1vrgibka-clang-unwrapped-with-lib/bin/$(basename $0) "$@"

$ ls /nix/store/nzyxg89vqxhdw884n50c2p5k1vrgibka-clang-unwrapped-with-lib/bin/
clangd

As you can see, clang-format is a symlink to clangd, which is a script, which calls $(basename $0) (being clang-format) from some unwrapped directory which doesn't contain the binary. I don't think I've broken this in my hacked up version.

Also note that paths passed to the CPATH / CPLUS_INCLUDE_PATH directories are somehow pointing to some clang-tools version 17 store paths. But this may be something I've caused by my crude use of the PR code.

[Patryk27]

I don't think I've broken this in my hacked up version.

You're right, my patch breaks this (including other tools like clang-move) - I'm looking into it now.

As for the overall approach to the fix, I'm not 100% sold on it either - I just cobbled together something that works better than the current approach; I'm not sure why we need the CPATH and CPLUS_INCLUDE_PATH thingies 🤔

[Patryk27]

Alright, I pushed a new commit that uses a bit less hacky approach - clang-format etc. should work as well now. Let me know what you think!

I'm also open to other approaches, just don't have any other idea myself.

(btw, I'll squash the commits later - I've pushed it as a separate commit to make it easier to review)

[lukash]

Just loaded the changes, both clangd and clang-format seem to work, I'll report back if I find an issue. Thank you!

Regarding an overall better / "correct" approach, ideally you could work with clangd upstream to figure out a proper solution. FWIW they were very helpful in my bugreports. But I guess someone would need to analyze the situation on Nix side and approach them with a clear description of the situation and what Nix needs... I can chime in especially with the arm compilation side, but lack a general in-depth insight.

[iniw]

I'm personally happy with the changes here. Would like to see this merged :)

[alyssais]

Can you please squash the commits and update the commit message for the new approach? Otherwise this is quite hard to review.

[alyssais]

The commit message doesn't explain anything at the moment. The details you put in the PR body would be a good start, but the addition of the lib symlink also requires explanation.

[iniw]

clangd 21 has a new BuiltinHeaders option that, when used alongside the unwrapped package, forms a better solution than my previous -resource-dir hack. So the workaround is to essentially:

• Use llvmPackages_21.clang-unwrapped
• Add the following to your .clangd:

CompileFlags:
  BuiltinHeaders: QueryDriver

[Patryk27]

ping ping 👀

[nixos-discourse]

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-in-distress/3604/124

[doronbehar]

I'm not a clang user, but I came after the "PRs in distress post", so I cannot review the changes to wrapper script and I haven't read thoroughly the comments. However these are my 2 cents:

It could be easier to review this if the changes were a bit more decoupled to these commits:

1. clang-tools: use finalAttrs
2. clang-tools: add passthru.tests
3. clang-tools: improve wrapper script & installPhase

In the 1st commit, I'd also remove the usage of ${finalAttrs.unwrapped} and the unwrapped argument to mkDerivation, in favor of simply ${clang-unwrapped} - it will be easier to figure out how to override it by looking at the expression.

[Patryk27]

@doronbehar thanks for taking a look!

In the 1st commit, I'd also remove the usage of ${finalAttrs.unwrapped} and the unwrapped argument to mkDerivation, in favor of simply ${clang-unwrapped}

In that case what should exactly remain in that commit? 🤔 (i.e. what would use those finalAttrs)

[doronbehar]

In that case what should exactly remain in that commit? 🤔 (i.e. what would use those finalAttrs)

Haha you are right :) If you have nothing to use finalAttrs with then maybe drop this commit.

[Patryk27]

Okie, done!

Eventually I've decided to leave the finalAttrs pattern as suggested previously - seems that most packages use it; other than that I've split the changes into separate commits.

[doronbehar]

Looks good I guess. Since there are many people who have reviewed this, and no body seems to have a strong objection, I think this should be good to go after the last small fix requested. Please write a message here if you think something in the PR is wrong.

[doronbehar]

In the last force push I only made the runCommand & writeText arguments be added in the 2nd commit that adds the passthru.tests - as only then they are needed. I am sorry I did this myself, I just felt bad how much this PR has been waiting.

[Patryk27]

I am sorry I did this myself, I just felt bad how much this PR has been waiting.

No problemo, thank you so much for taking a look and merging this!

[nixos-discourse]

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/dev-shell-flake-clangd-cant-find-gnu-stubs-32-h/51067/2