Windows!

[cpsauer]

Update: Windows support landed! Thanks all. The latest commits should have it. You can go ahead and start using this tool on Windows.

We haven't yet tested on Windows. Windows might need some patching parallel to that for macOS (in refresh.template.py), but it should be a relatively easy adaptation compared to writing things from scratch. If you're trying to use it on Windows, let us know here. We'd love to work together to get things working smoothly.

[lummax]

Just to leave a quick note while I am researching this:

• it seems we should not just do command = ' '.join(args) as the path to cl.exe contains whitespaces. I guess we could use https://stackoverflow.com/a/12130261
• /showIncludes and /P are the flags we are interested here
  • https://docs.microsoft.com/en-us/cpp/build/reference/showincludes-list-include-files?view=msvc-160
  • https://docs.microsoft.com/en-us/cpp/build/reference/p-preprocess-to-a-file?view=msvc-170
• I get a lot of warnings about missing header files from the C++ stdlib. https://docs.microsoft.com/en-us/cpp/error-messages/compiler-errors-1/fatal-error-c1083?view=msvc-170#the-command-line-include-or-lib-environment-is-not-set is a good pointe and indeed the bazel default toolchain sets these environment variables https://github.com/bazelbuild/bazel/blob/6dc941e58dfc1d4a9714a76b921fbe11fce658ed/tools/cpp/windows_cc_toolchain_config.bzl#L627. Currently I am not sure if the bazel aquery exposes that correctly. For some other action I see environmentVariables but not for the compile actions.

[cpsauer]

Hi, @lummax! Thanks for diving in here. Looks like good stuff to me. Tapping back some thoughts, in case they're helpful.

• Yep, I definitely should have considered shell escaping. Good call.
  • Alternatively, we could use the arguments array option in the compile_commands.json spec?
• 👍🏻
• Aha! Good find. Yeah, environment variables & clangd are tricky, especially since clangd doesn't know how to parse them from the specified command. You've probably already seen this in refresh.template.py, but I had a similar issue with the wrapper script for Apple platforms. I'd definitely suggest getting the environment variables from aquery if you can, and converting them into flags--sounds like you're already onto that. On Apple they weren't provided, so I extracted them from the system.
  • Just to double check: The search paths needed to find standard lib headers are indeed missing from the command, right? I just want to make sure that the issue isn't that clangd is failing to parse msvc flags.

Couple other notes from my mental cache that might be handy:

• There's this older project that I remember from the Implementation Readme. My memory is that it's windows/clang-cl specific, hasn't been updated in a while, and not, say, code we could really use, but it might be worth a quick look to see if it sparks good thinking about some additional edge case.

If there are other things where I could save you a bundle of time by answering questions or otherwise helping orient you in the code, please let me know. Communication lines are open, and I really appreciate your efforts.

[lummax]

On Apple they weren't provided, so I extracted them from the system.

I would like to avoid that. We are experimenting with using a vendored compiler toolchain where the complete configuration is already statically known in our bazel toolchain config. I guess it would make sense for the average user though :)

The search paths needed to find standard lib headers are indeed missing from the command, right?

It seems the search paths should be passed via environment variables. If these environment variables are not set header-extraction in extract.py fails as soon as the first stdlib header is included. But some included (project specific headers) are printed so there is at least some additional information.

clangd seems to inspect the environment or infers the correct stdlib paths from the cl.exe path. The problem I am having is localized to extract.py.

[lummax]

A completely different finding: https://github.com/bazelbuild/bazel/blob/09c621e4cf5b968f4c6cdf905ab142d5961f9ddc/src/main/java/com/google/devtools/build/lib/rules/cpp/ShowIncludesFilter.java#L58

When extracting the included headers from the cl.exe output, the extraction should be able to handle different languages it seems.

[cpsauer]

Gross. At least they all seem to end with a colon (58)? Still tough especially if it makes you recognize them in stderr.

(Ah! Didn't realize it was specific to refresh.template.py header finding. Clangd does indeed do some detection from the structure of known compilers. Hoping you find that the aquery output has the environment variables you need!)

[LoSealL]

Hi, I am happy to help make it work on Windows, and I init a PR to do so #25 .

And I'd like your help to figure out what else needs to be added there.

For now there are some known issues while using Bazel on Windows:

1. The bazel configuration seems not passed to the script template and it use the default config to do aquery and run. Hence the generated commands are all directed to x64-windows-fastbuild even the actual build happens in x64-windows-dbg.
2. The external bazel libraries' header paths (i.e. gtest) can't be get corrrectly from aquery command and because they are not installed, I don't know where to find them.

[cpsauer]

Wow--should I be so lucky as to have not one, but two efforts to add Windows support!

@LoSealL, thanks for your efforts and contribution. @lummax, though, is already quite far along with his work to add Windows support, including solving issues you raise. We're working on merging in the first part of his fix in #16, the other open PR, which moves everything into python so windows doesn't need a separate bat file!

If I may, and you'd still like to help, could I ask that you join forces? Perhaps there's a part where he'd like some help.

[LoSealL]

Oh, I'm not aware @lummax is working on Win support, just seeing a refactor PR opened. Thanks you for your reply.
I'd like to make my contribution if there are things to be done.

[lummax]

Sorry, I was slow to respond on the refactoring PR.

We are running https://github.com/axivion/bazel-compile-commands-extractor/commits/axivion (ignore the ugly git history) on Windows. Feel free to give that a try.

[LoSealL]

Hi @lummax, thanks for your great effort!
A little issue found:

• if system language (or more specific, VS language) is not utf-8 encoded (such as Chinese/Japanese), the subprocess.run commands will raise an exception about decoding error.
  Change code page to utf-8 and VS language to English could mitigate this issue, but can encoding='utf-8' be determined by system locale?

• Second one is the generated arguments are not quoted, and clangd may faile to execute the arguments.

     [
       {
         "file": "src/foo.cc",
         "arguments": [
           "C:/Program Files (x86)/Microsoft Visual Studio/2019/Community/VC/Tools/MSVC/14.29.30133/bin/HostX64/x64/cl.exe",
           "--language=c++",
          ...
         ]
       }
     ]

  What expected is "\"C:/Program Files (x86)/Microsoft Visual Studio/2019/Community/VC/Tools/MSVC/14.29.30133/bin/HostX64/x64/cl.exe\""

[LoSealL]

The include arguments for external packages are not correct

{
    "file": "src/foo.cc",
    "arguments": [
      "\"C:/Program Files (x86)/Microsoft Visual Studio/2019/Community/VC/Tools/MSVC/14.29.30133/bin/HostX64/x64/cl.exe\"",
      "--language=c++",
      "/nologo",
      "/DCOMPILER_MSVC",
      "/DNOMINMAX",
      "/D_WIN32_WINNT=0x0601",
      "/D_CRT_SECURE_NO_DEPRECATE",
      "/D_CRT_SECURE_NO_WARNINGS",
      "/bigobj",
      "/Zm500",
      "/EHsc",
      "/wd4351",
      "/wd4291",
      "/wd4250",
      "/wd4996",
      "/I.",
      "/Ibazel-out/x64_windows-fastbuild/bin",
      "/Iexternal/gtest",
      "/Ibazel-out/x64_windows-fastbuild/bin/external/gtest",
      "/Iexternal/bazel_tools",
      "/Ibazel-out/x64_windows-fastbuild/bin/external/bazel_tools",
      "/Ibazel-out/x64_windows-fastbuild/bin/spdlog/include",
      "/Ibazel-out/x64_windows-fastbuild/bin/fmt/include",
      "/Iexternal/gtest/googlemock",
      "/Ibazel-out/x64_windows-fastbuild/bin/external/gtest/googlemock",
      "/Iexternal/gtest/googlemock/include",
      "/Ibazel-out/x64_windows-fastbuild/bin/external/gtest/googlemock/include",
      "/Iexternal/gtest/googletest",
      "/Ibazel-out/x64_windows-fastbuild/bin/external/gtest/googletest",
      "/Iexternal/gtest/googletest/include",
      "/Ibazel-out/x64_windows-fastbuild/bin/external/gtest/googletest/include",
      "/DSPDLOG_FMT_EXTERNAL",
      "/DSPDLOG_WCHAR_TO_UTF8_SUPPORT",
      "/DSPDLOG_WCHAR_FILENAMES",
      "/showIncludes",
      "/MD",
      "/Od",
      "/Z7",
      "/wd4117",
      "-D__DATE__=\"redacted\"",
      "-D__TIMESTAMP__=\"redacted\"",
      "-D__TIME__=\"redacted\"",
      "/MT",
      "/Fobazel-out/x64_windows-fastbuild/bin/src/_objs/foo/foo.obj",
      "/c",
      "src/foo.cc"
    ]

For example the /Iexternal/gtest should be /Ibazel-<workspace_name>/external/gtest

[cpsauer]

Just merged @lummax's first PR! Thank you so much, Lukas!
I'll comment more in the PR in a second, but I'm excited to work with you on the ones this unblocked.
Let's go timezone tag-team.

@LoSealL, to respond to your three:

1. Encoding: This would be an awesome place for you to help, if you're willing! I know next to nothing about non-UTF-8 encodings, and it sounds like you know what's up. FWIW, I tried removing the encoding line, which should fall back to the system locale according to the docs, but it doesn't seem to work. Instead, it doesn't try to encode at all.
2. Quoting paths with spaces: This one I should have just fixed with a call to shlex.join. Credit to Lukas for showing me that package! (Sorry, @lummax: I think that raises the min python to 3.8, and you said you were currently running 3.7. Okay to upgrade?)
   Interesting that this works on Windows! Bazel categorically refuses to run on paths with spaces on other platforms, in my experience.
3. External symlink: Gah! No. this is a trap, and the output is right. bazel-<workspace_name>/external points into the execroot, which is super unstable and gets torn down and reconfigured on each build. You want to go through the output base.
   1. For now, please make sure you've got an external symlink, equivalent to the one described in the readme. [If that doesn't work or can't be done on Windows, please say something! My Windows knowledge is currently weak.]
   2. If you'd like to know more, please read https://github.com/hedronvision/bazel-compile-commands-extractor/blob/main/ImplementationReadme.md
   3. Lukas is working on auto-linking/rewiring options, I believe.

[LoSealL]

@cpsauer
For 1, I'm looking for how to deal with it, and once done, I'll make my PR again.
For 3, I missed the guide in Readme to explictily create a link for external folder. now I got it.

[cpsauer]

@LoSealL, any luck with any of the encoding stuff?
Also, any chance you have experience running vcvars to set msvc language and environment variables? No worries if not, but if you do, by any chance, we could use your help in #28

[LoSealL]

@cpsauer I see you have a new get_headers for msvc specificly, that's great!
So for encoding, just replace utf-8 by locale.getpreferredencoding().

For vcvars, you can refer to @bazel_tools//tools/cpp:windows_cc_configure.bzl where it has find_msvc_tool, find_vc_path that could tell you where vs installed.

[cpsauer]

Awesome :) Hoping I can ask @lummax to fold the locale fix into #28--or if not, we can do after or on merge. And on vcvars 👍🏻 (see note also in #28). Hoping, maybe, that you guys could collaborate on that if it's something you have experience with.

[redsun82]

hi there! I'm yet someone else interested in making this work on windows 🙂. I'm myself a windows newbie (I do my development in linux), but I'm the resident bazel expert of my cross-platform team and I want to help them out with IDE integration.

Ok, with presentations out of the way, I've tried the axivion:basic-windows-support branch out (with MSVC), and it mostly worked but for one small wrinkle: some of the include flags were spelled -I instead of /I in the resulting json, which resulted in missing headers. After I replaced those manually, the compilation database was working great with CLion 🎉

I can definitely look into it, but maybe someone of the participants to this conversation already knows what's going on there?

[lummax]

Hello, thanks for giving this a try :)

I did not notice something like that before. But as you, I am a Linux guy that is "the resident bazel expert of my cross-platform team" and I feel you.

Could this be due to some copts = ["-I..."] on some rules? We don't have those in our code base. I am not sure how bazel or msvc deals with a -I and that gets translated into a /I.

[lummax]

Seems there is some magic happening:

https://github.com/bazelbuild/bazel/blob/397f10812f7944ba45097c555940204cbac9dc98/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java#L688
https://github.com/bazelbuild/bazel/blob/397f10812f7944ba45097c555940204cbac9dc98/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java#L756

[redsun82]

indeed! nice catch!

yes, I do have explicit -I options in the build. As a temporary workaround I will look into either putting a select (we have those any way, with some nice macro wrappers), or I might also get away with strip_include_prefix in this specific case.

It might be worth though replicating that magic in the compilation database extractor maybe

[Arazu161]

So I'm not sure if this is a clangd issue, a VSCode issue, or an issue with the extractor or with Bazel, but for me, clangd isn't finding headers in directories specified with /I but it does work if I manually change the flag to -I (I'm testing with #28 ).

This happens with both llvm 13 and 14.

I was also wondering what the equivalent path for bazel-out/../../../external would be on Windows, as /../ does not work afaik

[redsun82]

So I'm not sure if this is a clangd issue, a VSCode issue, or an issue with the extractor or with Bazel, but for me, clangd isn't finding headers in directories specified with /I but it does work if I manually change the flag to -I (I'm testing with #28 ).

uh, that's weird, as for me it's the other way around... what compiler is it showing to use in the compilation db? I'm using CLion, so that might be a difference?

I was also wondering what the equivalent path for bazel-out/../../../external would be on Windows, as /../ does not work afaik

I did not have such an issue (but maybe I used \..\..\? I don't remember). In any case you can also use the output of bazel info output_base, and append \external to it

[cpsauer]

Hey, all! Merge of @lummax's good work coming shortly (should be tm). I've added some auto-junctioning and auto-git-ignoring goodies that should automate away all the manual stuff around the link, and still have it work automatically cross-platform.

More tomorrow :)

[Arazu161]

uh, that's weird, as for me it's the other way around... what compiler is it showing to use in the compilation db? I'm using CLion, so that might be a difference?

The compiler is cl.exe, BUT, I tried the same compile_commands.json in CLion and it complained about the string "'C:/Program Files (x86).... If I change it to "\"C:/..." so there are double quotes on either end of the path, Clion is happy. Back in VsCode, if I do the same, then clangd works with /I instead of -I

I did not have such an issue (but maybe I used ....? I don't remember). In any case you can also use the output of bazel info output_base, and append \external to it

The path works in Git bash, but not in Powershell/cmd, so the link can be set that way.

[cpsauer]

Friends, we're merged! Please grab the latest commit, and let us all know what you think. With the main part unblocked, it's incremental improvement time.

Huge thanks to @lummax for laying the Windows foundation and getting this rolling.

To make onboarding smoother and things more robust, I've added to his great work the auto-junction and auto-gitignore stuff mentioned above. There's also some additional Bazel toolchain robustness and a grab bag of miscellaneous fixes--all merged together.

To that end, you'll want to un-commit the //external symlink. First, unlink. Then stage. Then rerun the tool and let it patch your gitignore. Then stage and commit!

[cpsauer]

As part of all that, I resurrected an old, childhood Windows desktop and provisioned it for development. Looks like lots of us are in the same boat, cross-platform Bazel folks venturing together into windows

[cpsauer]

@LoSealL, I rolled in your proposed locale fix at the same time. Could you confirm that it works for you? Looking forward also to working with you on #35

[cpsauer]

I'm going to close this down for now, so we don't confuse people, but let's continue discussing the -I stuff here.

Also, hi @redsun82! Pretty cool to have a GitHub staff member here! If you're willing, I'd love to hear what you think of CLion (as opposed to, say VSCode), what brought you here, and how setup went. Same with you @Arazu161!

[cpsauer]

Could you all bring me up to speed a little more on this -I sub-issue?

I'm typing up what I'm putting together as I read. Please correct me if I'm wrong!

1. Some -Is accidentally snuck into a Windows build.
2. MSVC should be fine with that.
   (Microsoft says: "You may use either a forward slash (/) or a dash (-) to specify a compiler option." source)
   But I don't think anyone is reporting issues with this tool's MSVC-based header finding--or with being able to build. Instead the issues are with clangd's handling of those flags--or maybe also with CLion's combination of clangd and it's own autocomplete.
3. Clangd should therefore also be able to handle either argument form, but it sounds like there's some parsing incompatibility--maybe around quoting?

If that's right, would you guys be down to work together to track down what's breaking and report it to clangd? Those folks are really great. And then, a workaround until their next release, it's pretty easy for us to do "magic" patches. We can just add a _windows_platform_patch parallel to _apple_platform_patch, referencing the issue.

LMK what you think, or if I'm on the wrong track.

[Arazu161]

I've changed over to the latest commit, and the issue is still there.

It seems to be a quoting problem, CLion complains about "'C:/Program Files (x86)/Microsoft Visual Studio/2019/Community/VC/Tools/MSVC/14.29.30133/bin/HostX64/x64/cl.exe' and won't parse the compile commands file. VSCode (or the clangd extension) doesn't complain about the path, but I'm guessing it fails to recognise the compiler (it breaks on the space and doesn't reach the end of the string) and defaults to expecting -I instead of /I (gcc mode)

Changing the path in the command to "\"C:/Program Files (x86)/Microsoft Visual Studio/2019/Community/VC/Tools/MSVC/14.29.30133/bin/HostX64/x64/cl.exe\" fixes the error in CLion, and VSCode goes on to recognise both -I and /I commands (on a side note, it looks like cmake outputs -I)

A quick test with neovim showed the same problem

I would this is a clangd issue. I'm not familiar with the it's codebase, but from what I can see it expects single quote strings to be escaped, which isn't valid JSON (and on a quick test it looks like "\'C:/... does work with clangd, but you get a JSON error reported in VSCode). The issue shows itself on Windows, as on other platforms I'm guessing you're pretty unlikely to have a space in the path to your compiler.

I've opened an issue on clangd (clangd/clangd#1094) but, for general compatibility I feel like it would be better to specify the path with escaped double quotes ("\"C:/..../cl.exe\"). Cmake actually gives the path unquoted, without spaces (C:\\PROGRA~2\\MICROS~2\\2019\\Community\\VC\\Tools\\MSVC\\14.28.29910\\bin\\Hostx64\\x64\\cl.exe) but this is down to a cmake vs bazel difference rather than this tool

[sam-mccall]

Just to echo what I said on the clangd issue, the llvm libraries try to emulate the way that the platform's shell would parse this string, and cmd.exe doesn't support single quotes.

If you want to avoid quoting issues, setting the arguments array instead of command string avoids a lot of confusion here.

[cpsauer]

Thanks @sam-mccall and @Arazu161 tracking it down and being great. Really appreciate you both.

Arguments array it is. Just pushed a fix.
Looks like that's now the preferred way to do a compile_commands.json anyway.
[@lummax earns the right to tell me "I told you so", should he want :) ]

Bazelers, could you update and check that this works for you, too?
(@Arazu161, let's get you on the tip of main, so, e.g., the --language stuff is fixed and gone.)

[LoSealL]

Yes, the shlex escpae commands with space by single quote, while Windows command line (cmd, specifically) can't recognize it.
I think on Windows either we guide users to use powershell directly and call python subprocess through powershell, or replace shlex and espace commands with space by double quote.

[cpsauer]

Try the latest 😉

[Arazu161]

Looks good to me so far, thanks for putting in the work!