-
-
Notifications
You must be signed in to change notification settings - Fork 392
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
Generated compilation database compiler argument missing extension breaks compilation on Windows. #1286
Comments
Hm, one other solution to this (which I use), is to whitelist the Arduino compilers with clangd, e.g.:
This allows clangd to query the compiler for its internal include paths directly. Maybe a bit hardcoded, but it works well in practice. As for adding the include paths to the compilation database, I wonder if this is really common practice? The json format lacks a way to distinguish these includes from the actual include paths passed to the compiler, making the command listed in the database not the actual command that was ran. I guess that even if clangd manages to query the compiler, the worst case would be a duplicate system include path, which should be harmless, but still. Also, I'm not entirely sure how we can do this portably and reliably: arduino-cli does not necessarily know how a compiler commandline looks exactly (it might not even be gcc...). There's a few challenges I can see:
So I wonder if we can do this reliably at all, or if we should just stick to listing the actual commands that were ran and rely on clangd to figure out the rest with |
I also wonder: You paste a |
If this works, I agree this would be the optimal solution as the work is being done from the compiler. It's unfortunate that this isn't something easily configurable/you have to know about it. And I realize that this may be off-topic at this point, but it could potentially help another person trying to use clangd with VSCode and Arduino. I am not able to get this to work under Windows 10 x64. I have my .vscode/settings.json:
And it does not work. I suspect it may have something to do with the generated
I also tried with the command you pasted:
That is true, I do think that this functionality is as you said part of the compiler. That would make more sense.
The I personally much prefer the clangd extension for VS Code as I am used to it, it has arguably better, more modern intellisense, and also includes clang-format and clang-tidy. the clangd extension for VS Code does not interact with the The |
I created a barebones VS Code project here to test the driver configuration. In this project you would see that clangd intellisense says And I can see in the clangd output that the --query-driver argument is being passed in:
I added
Yep, when I correct the last few separators:
clangd will output:
Now the question is: "Is the CLI the one generating these incorrect paths in the Windows compilation database or is it the VSCode Arduino extension after it calls the CLI?" Oh @matthijskooijman! I think that you can either specify the driver in the I have confirmed that calling the CLI directly generates these incorrect paths (It is not the Arduino extension doing this):
So I believe this is actually a bug with the Arduino CLI rather than a feature request. Edit 2: Interestingly, after changing the generated
|
So to clear something up, @matthijskooijman, the I see now, when I remove the whitelist (which does not work on windows because of path mismatches from the arduino-cli-generated database) the following output:
But nowhere in there are the C++ includes from the toolchain Which makes sense, because if you look at the
It is all lowercase, which leads me to believe that the extension is adding these on to the compiler driver output. If I add those last include directories as shown below:
Then intellisense is happy. Note: all of those includes should be |
@Falven, ok so for the query driver option, the slashes are problematic? Did you try using only backward (or only forward) slashes in query-driver and manually modifying compile_commands.json to match? Does it work then? If so, then I guess the mixed slashes in compile_commands.json prevent clangd from working and should be fixed in arduino-cli. I think that for executing a command, these mixed slashes are acceptable on Windows, so there has not been any need to fix them so far. The mixed slashes come from the fact that some paths are autofilled (so use
I do not understand what you mean here? AFAIU, the idea is that --query-driver should match the compiler command listed in compile_commands.json and then clangd will query the compiler from compile_commands.json for all needed options. It might be that it does this by asking the compiler for the driver used and then querying the driver for options maybe, but all this is handled by clangd and AFAIK you only need to whitelist the outer compiler. |
@matthijskooijman I know I gave a lot of information throughout my debugging, so I apologize for that. From the clangd thread I linked, @kadircet says: What I gathered from this, then, is the following: the --query-driver option is not necessary, and only works as a whitelist. If there are multiple compilers in one toolchain, you can tell clangd which one to use from the toolchain. As @kadircet mentioned, The real driver path is deduced from the compile command in the
What we can then deduce is that if you don't provide the --query-driver option, clangd will still query all provided compilers in the I think the mixed slashes are a potential issue for certain applications. I think they are the underlying issue with clangd, for example, if you are trying to use this --query-driver option with the clangd CLI, it will not consider:
and
to be the same compiler. As can be observed from my previous comments. |
First of, as for terminology, I just realized that the command from compile-commands.json is actually the driver command, which internally executes the compiler (I think I might have had them reversed in my earlier comment). And clangd needs to figure out the compiler command used internally, since that includes all include paths explicitly. Below, I'm using the correct terms.
I do not think this is true. I believe that the query-driver option is a security feature, since clangd needs to execute the driver to query it, so it only does that for some known-safe driver paths, plus explicitly whitelisted ones with query-driver. If you do not specify query-driver and you end up with a driver in compile_commands.json that is not (default) whitelisted, I suspect that clangd will just assume you're using some default driver (i.e. /usr/bin/gcc, or maybe clang or so) and query that, or just autogenerate a default compiler commandline and includepaths.
I don't think this is entirely true: From your earlier comment, I see that if you specify all But if you change the
So that suggests to me that clangd indeed does not like the mixed slashes, but the problem is not in the whitelist, but in the execution of the driver. So, the proper fix is probably to produce a compile_commands.json with consistent slashes. Which is why I suggested you test this fix manually, i.e. from my previous comment:
|
Paying attention to slashes, we can see that:
outputs
and
outputs
I think the last output is a good hint. I think the problem then is that the compiler doesn't have these system include paths to extract defined in it's "driver".
Edit: Maybe this will be a hint, but I was following this issue which seems to be similar. And the output from the suggested command is the following:
It looks like it is ignoring all of the c++ include directories that we are missing. |
I don't know what you mean by this exactly, but I think it is wrong. What this means is that clangd tries to run the driver to query it for include paths and fails, because it does not support forward slashes in the driver executable path.
Do you mean the "ignoring duplicate directory" messages? Those are normal and just indicate that it's going to output these directories just once instead of multiple times. As you can see, they are output after the As I suggested before:
This is the third time I've given the above suggestion, which I think is the way forward to a fix, but it seems you're ignoring it, so I'm slowly running out of patience here. If my suggestion is unclear, please let me know, but otherwise please try this. |
@matthijskooijman It is not my intention to ignore anything, my apologies. I have modified the generated
I have set the query driver argument as such:
And the relevant output from clangd follows:
|
Hm, that surprises me, so just fixing the slashes is not going to help here, it seems. Also, it seems you're editing your github comments a lot, which sometimes makes it a bit hard to reply, since I often read messages from the e-mail notification, which shows the initial version. Maybe you could use the preview function a bit more, and if you do have significant new findings, just post a new comment? In the initial version of your last comment, the clangd error was different (does not exist rather than not allowed driver), what's up with that? I wonder if maybe vs is messing up the query driver argument somehow or something else? Maybe you could try running clangd manually directly to rule that out? |
Yeah, sorry. I made a typo - I had added .exe to the Let's try running it manually:
Now I have to figure out how to send it a file open command over stdout? |
Wait, so when you add .exe to the query-driver argument, it says "does not exist", while if you omit it, it says "not allowed"? That's weird, since I expect that it checks the whitelist first and then checks for existence of the file, so that would mean that "does not exist" means that the whitelist passed (so that would mean that internally it adds the .exe to the command from comipile_commands.json and checks that against the whitelist). Unless it strips the .exe from the whitelist and compares that, but that means that adding or removing the .exe form the whitelist should not affect the result and the error message. Or, unless the existence check is done before the whitelist, but then changing the whitelist should not change the result of the existence check (i.e. change a "does not exist" into a "not allowed"). I'm thoroughly confused and running out of ideas....
Ah, right, you'll have to send it a file to start doing something, that might not be so easy... |
Yeah, I couldn't figure out how, but you can see that argv[2], the |
On Linux, using arduino-cli and clangd running inside Vim with the YouCompleteMe plugin, it indeed works. I tried using your clangd-arduino.ino sketch:
The compile_commands.json for the .ino file looks like this:
clangd output is:
Note that if I drop the arduino paths from the query-driver option, I get the whitelist error as expected:
|
Thank you. What happens when you run:
|
I have made some progress, @matthijskooijman If I fix all of the non-windows paths in the generated
@matthijskooijman seems we have an actionable fix. :) From @kadircet
|
Ah, cool, so the .exe is apparently essential. That sounds like there's two changes to make:
Of these, 1. is probably easy, but 2. is a bit harder, because arduino-cli has no idea which arguments are pathnames and which use literal slashes. For clangd it is probably ok to just convert all slashes, or maybe specialcase this to only options starting with IIUC you're saying that just fixing 1. is not enough and fixing 2. is also needed? I suspect that 1. is enough to fix the whitelist issue and have clangd call the driver, but maybe 2. is also needed to make it understand the additional include paths? And I was indeed going to suggest fixing this at the clangd side, it would be good if it is more tolerant about omitting/adding .exe. And given that gcc also understands the mixed-slashes in its arguments, it would be nice if clangd would do the same. I'll provide a response to that issue as well in a minute. |
@matthijskooijman I'm sorry. I should have been more specific. I have just verified, you only need to fix:
That coupled with the windows-style path and extension in the This will do both, whitelist the driver, and have clangd call it allowing it to extract the missing C++ include paths. Note: This fix will work for my particular use case with clangd. I am unsure if the other mixed paths in |
Cool. Then one more question: If you just add the .exe to |
That worked as well.
So it seems the culprit is really only the missing exe file extensions and mismatched paths between the |
@matthijskooijman Edit: |
@matthijskooijman When compiling a simple sketch, such as the following:
Before system includes were fixed on Windows, at least for generated compile_commands.json, compile commands looked like the following:
These, of course, succeed in creating object files. However, now that the driver properly extracts system includes, compile commands would look like the following:
This causes issues when including any header-only or template libraries, including the STL.
It almost seems like it is being compiled as C code, even though NOTE: I understand the issues behind heap fragmentation and SRAM, Intellisense engines will use these commands to provide compiler output, so while people shouldn't be needing to execute these commands manually to create the object files, etc, the intellisense engines will add Any ideas what is going on here? Are the system includes somehow being included twice? Search results seem to suggest it is something with the include order and Edit:
Apparently it is a bug with this version of the compiler from 2017. Will we ever see a fix/update? A proposed solution that does not involve updating the compiler would have me wrap all of my c++ includes in See here for an issue I raised to try to update the toolchain. |
I'm having the same issue in macOS with arduino-cli 0.33.1. I'm using Emacs (with the eglot LSP client) with clangd. I get this error:
I fixed it by manually adding this to the
This is a long thread that I'm having some trouble following, so is the accepted solution to use the |
Bug Report
The generated compilation database has a bug/is missing some compiler commands.
If you are trying to use the compilation database as intellisense configuration as is done in pretty much any IDE, for example, clangd under VSCode, the generated compile_commands.json MUST explicity include the system header include directories otherwise clangd or other intellisense engines cannot find them. This is because Arduino uses different compilers in non-standard locations, with different system includes per compiler, as such the intellisense engines will not know where to find the system includes.
Current behavior
Here is an example file entry for the generated compilation database:
Expected behavior
Here is what the file entry from the compilation database should look like:
Environment
Additional context
As an example, look at the compilation database, c_cpp_properties.json, that the Arduino vscode extension generates for it's intellisense configuration. They add on the system include directories.
The text was updated successfully, but these errors were encountered: