search

hedronvision / bazel-compile-commands-extractor

Goal: Enable awesome tooling for Bazel users of the C language family.
Other
662 stars 108 forks source link

Custom-control-args #122

Open helly25 opened 1 year ago

helly25 commented 1 year ago

Add flags for custom output control.

Unlike in other PRs here all flags are prefixed with '--bcce-' (for Bazel-Compile-Command-Extractor) so they can easily be distinguished and filtered out.

cpsauer commented 1 year ago

Hi, Marcus! Great to meet you. Thanks for your high-quality PR--and for being the kind of person who uses his talents to leave things better than he found them. Sorry I've been slower than you deserve; I'm doing my best to catch back up after a trip. Love the idea of helping people run this tool automatically.

cpsauer commented 1 year ago

Since the only thing better than great flags is (also) automatically doing the right thing, I wanted to start by checking in with you on which of these we might be able to automate.

Starting with color: What do you think about automatically disabling color if the output isn't a terminal, parallel to this implementation, but augmenting its inspection of the TERM environment variable to match bazel's? That'd also get us standard manual override with, e.g. NO_COLOR, perhaps removing the need for the flag. (Some other variants that crossed my mind, which seemed worse, but which I'll still list for your consideration: We could also check the COLORTERM variable, but I don't know that we really need it. We could try to support the same flag as Bazel so our color is in sync with it, trying to inspect bazelrc with --announce_rc, but I think it's not worth it--I'd be inclined to just match the standard environment variables. What a shame that the vscode output pane doesn't support color!)

cpsauer commented 1 year ago

For argument addition and compiler override: I'd love to backtrace the need here--to understand what the problematic values are that you want to override. Ideally, of course, we'd automatically get good, de-bazeled commands for you, as we do with, e.g. unwrapping on Apple and the other platform patches. (I'm confident we shouldn't recommend that everyone just always override with clang, though; that'll break things, for example, for folks who are cross compiling.)

Some other quick notes:

Anyway, that's my quick look. Thanks again for being great :) and bring your experience to bear on this. Chris

helly25 commented 1 year ago

Thanks for the response Chris, I like stuff to work automatically while allowing users control in case the automation is wrong. So I looked at automatic color detection while keeping a color flag that defaults to 'auto', very similar to bazel's way.

Looking at the environment from the OUTPUT I get: VSCODE_AMD_ENTRYPOINT=vs/workbench/api/node/extensionHostProcess VSCODE_CRASH_REPORTER_PROCESS_TYPE=extensionHost VSCODE_CRASH_REPORTER_SANDBOXED_HINT=1 VSCODE_CWD=/ VSCODE_HANDLES_UNCAUGHT_ERRORS=true VSCODE_IPC_HOOK=/Users/marcus/Library/Application Support/Code/1.78-main.sock VSCODE_L10N_BUNDLE_LOCATION= VSCODE_NLS_CONFIG={"locale":"en-us","osLocale":"en-us","availableLanguages":{},"_languagePackSupport":true} VSCODE_PID=23867

But no TERM or similar var.

From the "Terminal" I get:

LANG=en_US.UTF-8 TERM=xterm-256color TERM_PROGRAM=vscode TERM_PROGRAM_VERSION=1.78.2 VSCODE_GIT_ASKPASS_EXTRA_ARGS=--ms-enable-electron-run-as-node VSCODE_GIT_ASKPASS_MAIN=/Applications/Visual Studio Code.app/Contents/Resources/app/extensions/git/dist/askpass-main.js VSCODE_GIT_ASKPASS_NODE=/Applications/Visual Studio Code.app/Contents/Frameworks/Code Helper (Plugin).app/Contents/MacOS/Code Helper (Plugin) VSCODE_GIT_IPC_HANDLE=/var/folders/lm/vprqmglx6zb56y6bx6wh23k80000gn/T/vscode-git-f92ff67fca.sock VSCODE_INJECTION=1

So we can check TERM and NO_COLOR for automation.

As for compiler and option overrides, I will add further explanations. But there is a difference between using cland's config and overriding the values in the generated compile_commands.json file. They simply have different used cases.

On Fri, Jun 2, 2023 at 5:39 AM Chris Sauer @.***> wrote:

For argument addition and compiler override: I'd love to backtrace the need here--to understand what the problematic values are that you want to override. Ideally, of course, we'd automatically get good, de-bazeled commands for you, as we do with, e.g. unwrapping on Apple and the other platform patches. (I'm confident we shouldn't recommend that everyone just always override with clang, though; that'll break things, for example, for folks who are cross compiling.)

Some other quick notes:

  • If this is for clangd, and we really want to manually override, I think you can also probably override both with a user level clangd config https://clangd.llvm.org/config, if that'd suit your needs. (Project-level clangd config will fail for out-of-tree, system headers, I'm guessing.) Might be an alternative to adding flags here.
  • This is super minor, but I think we also might want to tighten the emeraldwalk.runonsave regex just a little. I'm guessing it's matching the file path, right? So we probably want an ending $, can drop the .*s. And I think there are a couple more we might want to match, like .star. Quick search from the vscode plugin https://github.com/search?q=repo%3Abazelbuild%2Fvscode-bazel%20bzl&type=code .
  • Heads that there's a new feature probably coming (in a PR) soon that'll incrementally get/update commands for individual files. The idea is that it'd be called every time a file was opened in an editor, thereby accommodating folks with very large codebases where running the full extraction would be overwhelming.

Anyway, that's my quick look. Thanks again for being great :) and bring your experience to bear on this. Chris

— Reply to this email directly, view it on GitHub https://github.com/hedronvision/bazel-compile-commands-extractor/pull/122#issuecomment-1573097051, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABQ7NSPSL7KBNOFICAHRWP3XJFN5JANCNFSM6AAAAAAYKQCJQE . You are receiving this because you authored the thread.Message ID: @.*** .com>

cpsauer commented 1 year ago

Right there with you on "automatic but configurable" :) Thanks for your reply.

Would still love to backtrace your needs on the other two--for my understanding if nothing else! (And I think I should ask if you'd still follow up on some of the other remaining details above, if that's not too annoying--maybe you just haven't gotten to them yet.)

Cheers and thanks! Chris

helly25 commented 1 year ago

Here is how I ended up needing to control the compiler:

external/local_config_cc/cc_wrapper.sh --version
Apple clang version 14.0.3 (clang-1403.0.22.14.1)
Target: x86_64-apple-darwin22.5.0
Thread model: posix
InstalledDir: /Library/Developer/CommandLineTools/usr/bin

I can set the correct compiler to use with my clangd setup in the .clang project file or a user/computer wide control file, say:

CompileFlags:
    Compiler: /usr/local/opt/llvm/bin/clang

Unfortunately I cannot just say Compiler: clang as it will find the wrong one then, so I have to actually provide the absolute path.

Now the absolute path won't be the same for all users and all setups, so it cannot be shipped in the project's .clang file. On the other hand the extractor flag --bcce-compiler="$(which clang)" will work at least for any setup that uses clang. In particular this will work for projects that are invested in clangd or for projects that use https://github.com/grailbio/bazel-toolchain.

I could set the path correctly in a user-clangd settings file, but that could interfere with other projects if cross compile is in effect.

So it appeared to me that the compiler override is fairly useful, even if only for edge or complex cases. And with that in mind I added the ability to provide additional flags as well. However I left out the ability to strip compiler flags as one can often just provide more clags to override prior flags.

The most useful case for adding flags is actually to define or undefined macros when the code gets sent to clangd. For instance, if you switch between opt and dbg modes you might want to always enable debug symbols by providing the debug macro.

Hope this clarifies why I added --bcce-compiler and --bcce-copt flags.

cheers marcus

On Sat, Jun 3, 2023 at 2:26 AM Chris Sauer @.***> wrote:

Right there with you on "automatic but configurable" :) Thanks for your reply.

Would still love to backtrace your needs on the other two--for my understanding if nothing else! (And I think I should ask if you'd still follow up on some of the other remaining details above, if that's not too annoying--maybe you just haven't gotten to them yet.)

Cheers and thanks! Chris

— Reply to this email directly, view it on GitHub https://github.com/hedronvision/bazel-compile-commands-extractor/pull/122#issuecomment-1574486715, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABQ7NSPEGFE4FLB3CV4G3N3XJKAEJANCNFSM6AAAAAAYKQCJQE . You are receiving this because you authored the thread.Message ID: @.*** .com>

cpsauer commented 1 year ago

Hey, Marcus! Sorry--there's still something I'm still not understanding. (And I need to make sure I understand needs before we extend the interface.) Won't $(which clang) will give the same result as clang? Looks like the wrapper is around macOS CommandLineTools's clang, but that the other path is (maybe) for brew-installed-llvm clang?