Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sysroot handling is wrong in certain cases #179

Closed
alkis opened this issue Mar 28, 2024 · 3 comments
Closed

sysroot handling is wrong in certain cases #179

alkis opened this issue Mar 28, 2024 · 3 comments

Comments

@alkis
Copy link

alkis commented Mar 28, 2024

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:

# Swap -isysroot for --sysroot to work around some unknown sysroot bug in clangd.
# For context, see https://github.com/hedronvision/bazel-compile-commands-extractor/issues/82
# The = logic has to do with clang not accepting -isysroot=, but accepting --sysroot=. Note that -isysroot <path> is accepted, though undocumented.
compile_args = ('-isysroot'+arg[len('--sysroot')+arg.startswith('--sysroot='):] if arg.startswith('--sysroot') else arg for arg in compile_args)

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?

@alkis alkis changed the title sysroot handling is wrong sysroot handling is wrong in certain cases Apr 2, 2024
@cpsauer
Copy link
Contributor

cpsauer commented Apr 4, 2024

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
Copy link
Author

alkis commented Apr 4, 2024

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
Copy link
Contributor

cpsauer commented Apr 5, 2024

Yay! Delighted to hear it :)

@eschumacher-s confirmed over in #82 (comment) 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.]

cpsauer added a commit that referenced this issue Apr 5, 2024
@cpsauer cpsauer closed this as completed Apr 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants