Skip to content
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

Custom-control-args #122

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Conversation

helly25
Copy link

@helly25 helly25 commented May 22, 2023

Add flags for custom output control.

  • --bcce-color[=no] A boolean flag that enables or disables colored output. This is useful for environments where the color codes are not handled (e.g. VSCode output window).

  • --bcce-compiler[=compiler] Allows to override the detected compiler. This is helpful if the compiler found in the editor environment is different from the compiler that should be used for compile_commands.json.

  • --bcce-copt[=option] Enables passing additional options to arg lists in compile_commands.json (can be repeated).

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.

helly25 and others added 5 commits May 22, 2023 13:24
… that need custom command line options.

We add the following options that are all prefixed by `--bcce-` (for Bazel-Compile-Command-Extractor):

--bcce-color allows to enable/disable colored output.
--bcce-compiler allows to override the compiler (this goes beyond the current apple patching).
--bcce-copt allows to pass down additional args to the arg lists.
@cpsauer
Copy link
Contributor

cpsauer commented Jun 2, 2023

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
Copy link
Contributor

cpsauer commented Jun 2, 2023

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
Copy link
Contributor

cpsauer commented Jun 2, 2023

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, 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.
  • 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

@helly25
Copy link
Author

helly25 commented Jun 2, 2023 via email

@cpsauer
Copy link
Contributor

cpsauer commented Jun 3, 2023

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
Copy link
Author

helly25 commented Jun 3, 2023 via email

@cpsauer
Copy link
Contributor

cpsauer commented Jun 9, 2023

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants