hedronvision / bazel-compile-commands-extractor

Goal: Enable awesome tooling for Bazel users of the C language family.
Other
659 stars 109 forks source link

sysroot handling is wrong in certain cases #179

Closed alkis closed 5 months ago

alkis commented 5 months ago

Thanks for the great tool!

I have an issue where in my compile_commands.json the sysroot argument comes out garbled, like this:

      "-isysrootexternal/sysroot-linux-x86_64-gnu7-llvm-16",

This snippet here seems a bit weird:

https://github.com/hedronvision/bazel-compile-commands-extractor/blob/5bcb0bd8a917b2b48fb5dc55818515f4be3b63ff/refresh.template.py#L873-L876

I think the comment is saying that we are rewriting -isysroot to --sysroot but the code does the reverse. Plus this:

    compile_args = ('-isysroot'+arg[len('--sysroot')+arg.startswith('--sysroot='):] if arg.startswith('--sysroot') else arg for arg in compile_args)

will rewrite --sysroot=XXX to -isysrootXXX which is likely what's happening in my case. If we want to generate -isysroot XXX here we would need to generate twice as many input args start with --sysroot, right?

cpsauer commented 5 months ago

Hi again, Alkis! It's great to hear from you. Tool otherwise working okay for you guys at Databricks?

Definitely want to check with you, but I think that's working as originally intended (though my comment is at best ambiguous....)--though it's well possible that my original intent was wrong. Is it causing downstream errors?

The reason it's using the -isysroot<dir> form is that that's the only one documented by clang.

I'll go ahead and disambiguate the comment at the very least. It's possible clangd has fixed the underlying bug there... I'll check in on #84, but if you're using GCC, could I ask you to try removing that patch line and see if all still works successfully?

alkis commented 5 months ago

The tool is working great at Databricks. Thank you Chris!

We are using clang. Regarding sysroot support in clang, I am not an expert but --sysroot seems to be the right way to do it according to https://clang.llvm.org/docs/CrossCompilation.html. Also --sysroot is documented in the documentation linked above https://clang.llvm.org/docs/ClangCommandLineReference.html#cmdoption-clang-sysroot, so perhaps we are fighting an old bug here? Maybe we should remove the code?

In any case the current code generates -isysrootXXXX. That is no space or = between flag and value. That seems wrong, no?

cpsauer commented 5 months ago

Yay! Delighted to hear it :)

@eschumacher-s confirmed over in https://github.com/hedronvision/bazel-compile-commands-extractor/issues/82#issuecomment-2036445198 that the underlying clangd bug seems to be resolved, so I'll go ahead and remove the workaround.

[That said, I still think this was working right? Bug was in clangd, not clang. -isysrootXXXX was indeed the documented form, as above. sysroot and isysroot are quite similar.]