[Windows] Support --features=compiler_param_file

[LoSealL]

Build using --features=compiler_param_file would wrap all source files and flags into a .params file, I think we should support to read from the .params file instead of reading from command line directly.

[cpsauer]

Hello again, @LoSealL! Good to see you back :)

I'd love to understand a little more about what you're trying to solve with this. I think that's a Windows-specific bazel flag for command lines that are too long. Is that right?

[In my head, I'm imagining that we probably wouldn't need to work around command-line-length limits for clangd, right, since presumably clangd can read arbitrary length commands out of the compile_commands.json file--and we'd like everything self contained in that file?]

All the best,
Chris

[LoSealL]

Yes, this is the Windows-specific flag in Bazel, while it's also a recommended flag in Bazel's official document. So I think many people using Bazel on Windows would specify this flag (including me).

It doesn't affect clangd itself, but while parsing sources and headers in the python script, it will fail to read the true parameters because it only gets a parameter file, such as:

cl.exe --language=c++ @bazel-out/x64_windows-opt/bin/foo.obj.params"

So we need a patch to handle it, basicly to read the actual parameters in the file for later parsing.

Thanks.

[cpsauer]

Oh I see. (Same link, right?) Is the issue is just for header finding, then?

I'm mostly Linux/mac focused at the moment, as you probably know. So I'll need a bit of your help here.

Do you know if the max command length also exists for subprocess calls from Python?

I'm trying to scope whether it would it work to have this tool just (automatically) turn off that feature--or whether it'll be important to keep it on.

[cpsauer]

Windows compatibility hopefully landing soon. It's a bit stalled out because some things need to be a bit more robust before it lands IMO.

[LoSealL]

No, all the sources and headers will be lost.
Currently I'm tracking the Windows PR #28 and add all my supports there (maybe through my own PR after #28 was merged)

[cpsauer]

I really appreciate your care and willingness to help @LoSealL.

We'll definitely need to land #28 and then return to this. (I'm going to try to provision a Windows machine tomorrow.)

But in the meantime, do you know how we'd fix this? Same question as above: Could sidestep the issue by having the tool disable --features=compiler_param_file when running aquery? Or is it clear to you that we need to read (and write!) these files?

[LoSealL]

Yes, I know how to fix it, along with the locale problem. I am using a customized python script in my own project and testing it for several weeks, current fixes list below:

• Escape commands with double quotes
• Support reading sources and headers from parameter file (which I found is very effective because the update of command flags will immediately take effect on clangd!)
• Fix encoding problem with non-English codepage (Tested with Chinese [chcp 936])
• Can append to exsited compile_commands.json rather than overwrite it
• Automatically create a link to external.

[cpsauer]

Awesome!

[cpsauer]

Only non-awesome is that it looks like we duplicated some work bc I didn't know you'd done all that and we were slow on getting that branch merged. But could be worse! Multiple great people arriving at the same conclusions :)

Could I ask you to look at the latest and see what you can improve?

[cpsauer]

With these param files: I was just handling some cases for people where no build had yet been run since the last clean, so generated files were missing. Could I ask you to check those cases with params files?

[cpsauer]

More generally, thank you in advance! Sorry this has been slow and somewhat frustrating, but please know that I really appreciate your efforts.

You might also chuckle knowing that last night I set my visual studio to Chinese (which I do not read!) to test things :)

[LoSealL]

I found that most issues are resolved in the latest patch, great! @cpsauer
Some more issues found and listed here:

• When user is not build bazel project yet or called bazel clean, there will be no bazel-out folder.
• User may change the name of bazel-out
• Parameter file may not exist when related sources are compiled with error (Still possible to generate correct compile commands on an error source file?)

I will try to fix these on my own fork first, and make a new PR when it's ready

[cpsauer]

Thanks so much for investigating, @LoSealL!

I think let's not worry about the first two.
Why? bazel-out will reappear when the tool is run. Plus we need already need bazel-out to have that name for the source tree to have the same directory structure as the execution sandbox. (Which we assume on macOS and Linux. And I think problems are easily avoidable. More context in #14 if you want it.) If we do want to improve the second bullet, I think the best first step would be to have a more helpful error message when bazel-out is missing.

I'm more interested in the third bullet, where we're trying to generate a good and useful compile_commands.json, even when the code can't be compiled. I think it's fairly common that, during development, the code will be broken sometimes, so we don't want to require it to compile. We can definitely get useful commands with the compiler_param_file feature off, at least.

Thanks again!
Chris

P.S. I'm curious what motivated the append feature!

[cpsauer]

Hey, @LoSealL! Just wanted to touch base to see how things are going.

Cheers,
Chris

[LoSealL]

@cpsauer Didn't get much time on this, just submit my PR, please have a review #49

Thanks!

[cpsauer]

Thanks for posting, @LoSealL! I've read through your code.

Did you get a chance to try having the tool get the full, updated arguments without requiring a build by having the tool temporarily disable --features=compiler_param_file when it invokes bazel aquery? (As above, but maybe not said explicitly enough.) I'd think adding --features=-compiler_param_file to the aquery invocation might do it, based on this.

[Sorry about the whitespace issues that have crept in. Thanks for dealing with those. If you know of a good, e.g. bot to deal with them, I'd love to hear about it.]

[LoSealL]

A good point, will have a try.

Windows has a command line buffer limitation, some targets with huge source files and compile flags will be broken, but not sure how they behave under aquery

[cpsauer]

But since aquery doesn't actually run the command, won't we be okay?

[cpsauer]

[Though we might well hit the limit while invoking msvc to find headers. Assuming that's a problem, we could just write our own tmp command file, right, and still not require that the code build?]

[LoSealL]

@cpsauer Have a quick try, seems to work! Let me look into how to do it.

[cpsauer]

Yay! Thanks @LoSealL.

[One thing to keep in mind: if writing tmp files to find headers, we'll need to make sure the threads write to different files to avoid collisions.]

[cpsauer]

@LoSealL, just wanted to check in: Where are we at on this?
(I read your message as you trying that approach and finding it worked, but I wanted to make sure.)

Thanks!
Chris

[LoSealL]

@cpsauer See the updated of PR #49
I made the threshold of cmd line length to 8000, which theoretically is 8191.
It's workable for me, but it'd better to have more tests on other Windows environment.

Another question is should we clear the temp file that created in the script?

[cpsauer]

I was just reading though :) Thanks. (Github notified me when you pushed a new commit.)

Let's continue the discussion over there. I'll reply there in a sec.