microsoft / vscode-cpptools

Official repository for the Microsoft C/C++ extension for VS Code.
Other
5.52k stars 1.55k forks source link

API Provider does not respect clang-x64 intellisense mode on windows #2228

Closed ThadHouse closed 6 years ago

ThadHouse commented 6 years ago

I'm still working on more debugging, but with the update to 0.17.6 our include orders are weird. We have some users on gcc using Linux, and no matter how our include files are included (<> or "" ) the system included directories are searched before the added user directories. We have a library (opencv) that is both installed as a system library, but we want to use our own version. When we add an include, intellisense picks up the system one, which is wrong, and when compiling the user one is included.

Will edit with more details when I have done more digging myself and can get the logs.

bobbrow commented 6 years ago

This can be the behavior if you are using a recursive includePath (e.g. ${workspaceFolder}/**). The workaround is to force the folder with the duplicate header names above the recursive path like this:

"includePath": [
    "${workspaceFolder}/path/to/duplicate/named/headers",
    "${workspaceFolder}/**"
]
ThadHouse commented 6 years ago

I'm using the new extension API and definitely no recursive paths in what I'm sending.

bobbrow commented 6 years ago

Can you share the custom config object you are sending? Feel free to redact any sensitive info.

ThadHouse commented 6 years ago

Here is the config I am sending.

[
    {
        "configuration": {
            "compilerPath": "C:\\frc\\bin\\arm-frc-linux-gnueabi-g++.exe",
            "defines": [],
            "includePath": [
                "C:\\Users\\thadh\\Documents\\GitHub\\thadhouse\\allwpilib\\wpiutil\\src\\main\\native\\include",
                "C:\\Users\\thadh\\Documents\\GitHub\\thadhouse\\allwpilib\\build\\embeddedJniHeaders\\arm-linux-jni",
                "C:\\Users\\thadh\\Documents\\GitHub\\thadhouse\\allwpilib\\build\\tmp\\expandedArchives\\opencv-cpp-3.2.0-headers.zip_1b8591564854592a4aff6bd224b2b0a3",
                "C:\\Users\\thadh\\Documents\\GitHub\\thadhouse\\allwpilib\\build\\embeddedJniHeaders\\arm-linux-jni\\linux",
                "C:\\Users\\thadh\\Documents\\GitHub\\thadhouse\\allwpilib\\cscore\\build\\jniinclude\\compileJava",
                "C:\\Users\\thadh\\Documents\\GitHub\\thadhouse\\allwpilib\\cscore\\src\\main\\native\\include"
            ],
            "intelliSenseMode": "clang-x64",
            "standard": "c++14"
        },
        "uri": "c:\\Users\\thadh\\Documents\\GitHub\\thadhouse\\allwpilib\\cscore\\src\\main\\native\\cpp\\MjpegServerImpl.cpp"
    }
]

And here is the output from logging in Debug mode.

sending compilation args for C:\USERS\THADH\DOCUMENTS\GITHUB\THADHOUSE\ALLWPILIB\CSCORE\SRC\MAIN\NATIVE\CPP\MJPEGSERVERIMPL.CPP
  include: C:\PROGRAM FILES (X86)\MICROSOFT VISUAL STUDIO\2017\ENTERPRISE\VC\TOOLS\MSVC\14.14.26428\INCLUDE
  include: C:\PROGRAM FILES (X86)\MICROSOFT VISUAL STUDIO\2017\ENTERPRISE\VC\TOOLS\MSVC\14.14.26428\ATLMFC\INCLUDE
  include: C:\PROGRAM FILES (X86)\WINDOWS KITS\10\INCLUDE\10.0.17134.0\UM
  include: C:\PROGRAM FILES (X86)\WINDOWS KITS\10\INCLUDE\10.0.17134.0\UCRT
  include: C:\PROGRAM FILES (X86)\WINDOWS KITS\10\INCLUDE\10.0.17134.0\SHARED
  include: C:\PROGRAM FILES (X86)\WINDOWS KITS\10\INCLUDE\10.0.17134.0\WINRT
  include: C:\PROGRAM FILES (X86)\WINDOWS KITS\10\INCLUDE\10.0.17134.0\CPPWINRT
  include: C:\USERS\THADH\DOCUMENTS\GITHUB\THADHOUSE\ALLWPILIB\WPIUTIL\SRC\MAIN\NATIVE\INCLUDE
  include: C:\USERS\THADH\DOCUMENTS\GITHUB\THADHOUSE\ALLWPILIB\CSCORE\SRC\MAIN\NATIVE\INCLUDE
  define: _DEBUG
  define: UNICODE
  define: _UNICODE
  stdver: --ms_c++latest

Theres also another issue too. Its ignoring the compiler settings, and also not actually including all the directories I provide. Should I do those as 2 separate issues, or keep those in here for now?

bobbrow commented 6 years ago

I think the problem is your uri. It should be using the file scheme. The extension is probably err'ing out when you send the config so it doesn't apply it.

I should also add some logs to the debug spew for when custom configs are provided.

ThadHouse commented 6 years ago

The URI is actually passed as a string through the extension API. So I was assuming it took the fsPath of the passed in URI. Is that not the case? If so, that should be documented.

Also, I am not getting the message saying my extension cannot provide configuration for the file. Whereas if I do mess up that URI to point to a bad directory I do get the error message.

Below is where my code is set to setup the API.

https://github.com/wpilibsuite/vscode-wpilib/blob/master/vscode-wpilib/src/cppprovider/apiprovider.ts#L89

ThadHouse commented 6 years ago

But, if I do switch that from fsPath to toString(), it does semi work again.

sending compilation args for C:\USERS\THADH\DOCUMENTS\GITHUB\THADHOUSE\ALLWPILIB\CSCORE\SRC\MAIN\NATIVE\CPP\SINKIMPL.CPP
  include: C:\USERS\THADH\DOCUMENTS\GITHUB\THADHOUSE\ALLWPILIB\WPIUTIL\SRC\MAIN\NATIVE\INCLUDE
  include: C:\USERS\THADH\DOCUMENTS\GITHUB\THADHOUSE\ALLWPILIB\BUILD\TMP\EXPANDEDARCHIVES\OPENCV-CPP-3.2.0-HEADERS.ZIP_1B8591564854592A4AFF6BD224B2B0A3
  include: C:\USERS\THADH\DOCUMENTS\GITHUB\THADHOUSE\ALLWPILIB\CSCORE\SRC\MAIN\NATIVE\INCLUDE
  include: C:\FRC\ARM-FRC-LINUX-GNUEABI\INCLUDE\C++\5.5.0
  include: C:\FRC\ARM-FRC-LINUX-GNUEABI\INCLUDE\C++\5.5.0\ARM-FRC-LINUX-GNUEABI
  include: C:\FRC\ARM-FRC-LINUX-GNUEABI\INCLUDE\C++\5.5.0\BACKWARD
  include: C:\FRC\LIB\GCC\ARM-FRC-LINUX-GNUEABI\5.5.0\INCLUDE
  include: C:\FRC\LIB\GCC\ARM-FRC-LINUX-GNUEABI\5.5.0\INCLUDE-FIXED
  include: C:\FRC\ARM-FRC-LINUX-GNUEABI\INCLUDE
  include: C:\FRC\USR\INCLUDE

However, it is still missing some of my passed in include directories. The following are the ones passed in, and its missing half of them.

"C:\\Users\\thadh\\Documents\\GitHub\\thadhouse\\allwpilib\\wpiutil\\src\\main\\native\\include",
"C:\\Users\\thadh\\Documents\\GitHub\\thadhouse\\allwpilib\\build\\embeddedJniHeaders\\arm-linux-jni",
"C:\\Users\\thadh\\Documents\\GitHub\\thadhouse\\allwpilib\\build\\tmp\\expandedArchives\\opencv-cpp-3.2.0-headers.zip_1b8591564854592a4aff6bd224b2b0a3",
"C:\\Users\\thadh\\Documents\\GitHub\\thadhouse\\allwpilib\\build\\embeddedJniHeaders\\arm-linux-jni\\linux",
"C:\\Users\\thadh\\Documents\\GitHub\\thadhouse\\allwpilib\\cscore\\build\\jniinclude\\compileJava",
"C:\\Users\\thadh\\Documents\\GitHub\\thadhouse\\allwpilib\\cscore\\src\\main\\native\\include"

Also, it still doesn't recognize some files as matching, even though my api is definitely returning proper configurations.

ThadHouse commented 6 years ago

It also seems to be doing some parsing before updating the compiler args. As when walking through intellisense, _MSC_VER seems to be set, although in GCC its definitely not set.

ThadHouse commented 6 years ago

Sorry for all the messages. One thing that might be better before the API gets more commonly used is to change the uri parameter of SourceFileConfigurationItem to actually take a Uri object. Since that library already depends on vscode, that shouldn't be a problem, and then how that gets parsed doesn't matter. Especially since the Uri object is actually what gets sent to the get provider function.

bobbrow commented 6 years ago

Thanks for catching that. I will update the API to type it to vscode.Uri. Look for an updated version later today.

The error message only appears when a new file is opened. If you are providing multiple configs at once as a response to calling didChangeCustomConfiguration, there is no message.

If our extension loads before yours, it is possible that the wrong config will be sent at the beginning. When you register your extension as a provider, it might be safest to call didChangeCustomConfiguration as soon as you are ready to provide configurations so that IntelliSense can be reset. (though perhaps the extension should do this automatically)

ThadHouse commented 6 years ago

I do call didChangeCustomConfiguration. The thing that is making me think there is something weird with the parsing is that both _MSC_VER and __GNUC__ are both defined in the same file. The MSVC one is 1914, which is what my local MSVC is, and __GNUC__ is 5, which is also right for my compiler. And _MSC_VER is not in the defines list that the logging showed was passed for the file.

bobbrow commented 6 years ago

ok. i'll take a look.

ThadHouse commented 6 years ago

I'll try and build a more minimal repro when I have a chance. I am using a GCC linux cross compiler for windows, so thats how I am using GCC on a windows machine. If you want the exact compiler, it can be gotten here (the 2018 windows installer)

http://first.wpi.edu/FRC/roborio/toolchains/

However, any GCC cross compiler should work.

bobbrow commented 6 years ago

When cpptools sends _MSC_VER it sends 1914, so I'm not sure where the 1913 is coming from. A repro would be great.

ThadHouse commented 6 years ago

I was mistaken when I wrote 1913, it was sending 1914.

bobbrow commented 6 years ago

ok. It should only send that when intelliSenseMode is "msvc-x64', but there could be a bug here.

ThadHouse commented 6 years ago

https://github.com/ThadHouse/cpptoolsrepro

Heres a minimal repro. Open the debugging for the extension into the ReproductionFolder In the repo. You can either use the compiler I posted above, or your own cross compiler, and then change it in the extension. It defaults to the compiler posted above.

ThadHouse commented 6 years ago

@bobbrow quick question so I can commit the few changes that I made. I noticed that api version 1.3.0 was published. Is that still compatible with vscode cpp 0.17.6, or will I need to wait for an update?

bobbrow commented 6 years ago

Don't use it yet. I might have to revert it. I published it to start testing in our extension because it looked safe, but tests are failing and it looks like the serialization doesn't work as expected. For compat reasons, I might have to leave this property as string until the next major API revision (v2.0.0).

ThadHouse commented 6 years ago

Ok. So we'll just have to document to use .toString() on the passed in Uri to get it into the configuration that gets returned.

bobbrow commented 6 years ago

yep. I'm doing that now and adding a code example to the comment.

ThadHouse commented 6 years ago

Cool. And hopefully the repro posted above and help you figure out the other issue. Its making this unusable for building any programs with libraries.

bobbrow commented 6 years ago

1.3.1 is published and is now just documentation updates - no API changes. I'll try to take a look at your repro this afternoon.

bobbrow commented 6 years ago

I see the bug in our extension. The provided intelliSenseMode is not being used correctly so the translation unit is being compiled in msvc-x64 mode (the default for Windows) with GCC headers. That's not going to work... I'll get a fix out for the next version of the extension. Good find!

bobbrow commented 6 years ago

As for the missing paths, can you verify that they exist? We do have code that trims out invalid paths. We should probably log those to the debug channel too...

ThadHouse commented 6 years ago

They definitely do exist, because they're needed for compilation and the files do compile. If you can print out which paths get ignored, and why they get ignored I could probably find it quick. If thats on the language server side if you can get me a way to use a custom language server I'll happily test.

Any chance this update would come out sometime next week? Or should I remove it from my extension until then?

bobbrow commented 6 years ago

It seems the GCC/Clang on Windows scenario for custom providers is completely blocked by this bug, so I wouldn't recommend turning this on in your extension for Windows if this is a key scenario for you. You could leave it in for other OSes though. I'm contemplating adding a method to the API that will give you cpptools' version number so you can conditionally turn it off too. I'm wary of that though because I don't want people making versioning mistakes. The API might be of the form isVersionGreaterThan so that you can specify a minimum version requirement.

Our current release target is the week of July 16, so it won't be next week, but we could build an insiders VSIX and share that with you earlier to make sure everything works for you before the official release.

bobbrow commented 6 years ago

I forgot to mention that there is a way to make it work in the current version and I expect it may not be a problem if your users already have a c_cpp_properties.json or use the C_Cpp.default.intelliSenseMode setting. If intelliSenseMode is set to clang-x64 in either of those locations, then it works as expected.

ThadHouse commented 6 years ago

Our use case is almost exclusively GCC/Clang on windows, but good to know about the workaround.

Turns out the header issue was my fault. I was running a clean, and wasn't registering in my head that that deleted the directories I was looking for. So the clang-x64 issue is the only issue I currently see.

ThadHouse commented 6 years ago

I renamed this issue to actually match what the found issue in here was.

sean-mcmanus commented 6 years ago

Fixed with 0.17.7.