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

Bug: workerd compile_commands.json generation is flakey #506

Closed
ohodson opened this issue Apr 4, 2023 · 2 comments
Closed

Bug: workerd compile_commands.json generation is flakey #506

ohodson opened this issue Apr 4, 2023 · 2 comments

Comments

@ohodson
Copy link
Contributor

ohodson commented Apr 4, 2023

Generating compile_commands.json in workerd is non-deterministic.

This bug just highlights the problem and some initial observations.

Workerd is using https://github.com/hedronvision/bazel-compile-commands-extractor to generate compile_commands.json

Invoking bazel run //:refresh_compile_commands usually succeeds if invoked after first running bazel clean --expunge; bazel test //.... However, if launched at arbitrary times, invocation logs errors about missing files and defines.

Here is an example in my current tree:

$ bazel run //:refresh_compile_commands
INFO: Invocation ID: 36544ffc-7920-498f-a309-f8c4a2c13292
INFO: Build options --@rules_rust//:extra_rustc_flags, --cxxopt, --host_cxxopt, and 3 more have changed, discarding analysis cache.
INFO: Analyzed target //:refresh_compile_commands (1 packages loaded, 455 targets configured).
INFO: Found 1 target...
Target //:refresh_compile_commands up-to-date:
  bazel-bin/refresh_compile_commands.py
  bazel-bin/refresh_compile_commands
INFO: Elapsed time: 0.433s, Critical Path: 0.00s
INFO: 2 processes: 2 internal.
INFO: Build completed successfully, 2 total actions
INFO: Running command line: bazel-bin/refresh_compile_commands
>>> Analyzing commands used in @//...
>>> A source file you compile doesn't (yet) exist: bazel-out/k8-fastbuild/bin/src/cloudflare/cfbundle.capnp.c++
    It's probably a generated file, and you haven't yet run a build to generate it.
    That's OK; your code doesn't even have to compile for this tool to work.
    If you can, though, you might want to run a build of your code.
        That way everything is generated, browsable and indexed for autocomplete.
    However, if you have *already* built your code, and generated the missing file...
        Please make sure you're supplying this tool with the same flags you use to build.
        You can either use a refresh_compile_commands rule or the special -- syntax. Please see the README.
        [Supplying flags normally won't work. That just causes this tool to be built with those flags.]
    Continuing gracefully...
>>> While locating the headers you use, we encountered a compiler warning or error.
    No need to worry; your code doesn't have to compile for this tool to work.
    However, we'll still print the errors and warnings in case they're helpful for you in fixing them.
    If the errors are about missing files that Bazel should generate:
        You might want to run a build of your code with --keep_going.
        That way, everything possible is generated, browsable and indexed for autocomplete.
    But, if you have *already* built your code successfully:
        Please make sure you're supplying this tool with the same flags you use to build.
        You can either use a refresh_compile_commands rule or the special -- syntax. Please see the README.
        [Supplying flags normally won't work. That just causes this tool to be built with those flags.]
    Continuing gracefully...
In file included from bazel-out/k8-fastbuild/bin/src/workerd/jsg/modules.capnp.c++:4:
bazel-out/k8-fastbuild/bin/src/workerd/jsg/modules.capnp.h:10:2: error: "CAPNP_VERSION is not defined, is capnp/generated-header-support.h missing?"
#error "CAPNP_VERSION is not defined, is capnp/generated-header-support.h missing?"
 ^
1 error generated.
In file included from bazel-out/k8-fastbuild/bin/icudata-embed.capnp.c++:4:
bazel-out/k8-fastbuild/bin/icudata-embed.capnp.h:10:2: error: "CAPNP_VERSION is not defined, is capnp/generated-header-support.h missing?"
#error "CAPNP_VERSION is not defined, is capnp/generated-header-support.h missing?"
 ^

A partial fix for the errors above is to invoke bazel build -c fastbuild //... and then try again to leave just the warning:

>>> Analyzing commands used in @//...
>>> A source file you compile doesn't (yet) exist: bazel-out/k8-opt-exec-2B5CBBC6/bin/src/workerd/server/workerd.capnp.c++
    It's probably a generated file, and you haven't yet run a build to generate it.
    That's OK; your code doesn't even have to compile for this tool to work.
    If you can, though, you might want to run a build of your code.
        That way everything is generated, browsable and indexed for autocomplete.
    However, if you have *already* built your code, and generated the missing file...
        Please make sure you're supplying this tool with the same flags you use to build.
        You can either use a refresh_compile_commands rule or the special -- syntax. Please see the README.
        [Supplying flags normally won't work. That just causes this tool to be built with those flags.]
    Continuing gracefully...
>>> Finished extracting commands for @//...

The warning message likely points the way to a fix.

@ohodson ohodson changed the title Bug: compile_commands.json generation is flakey Bug: workerd compile_commands.json generation is flakey Apr 4, 2023
@kentonv
Copy link
Member

kentonv commented Apr 5, 2023

I'm just going to throw this in here: I really don't like our compile commands generator at all.

  • It gets out-of-sync whenever anything in the BUILD files changes and then I have to re-run it which takes forever.
  • When I run bazel targeting a specific test or build rule, suddenly the compile commands for all files that weren't needed by that specific rule stop working, and all those files are flagged as full of errors in VSCode. This continues until I build //... again.
  • Jump-to-definition tends to open some bazel symlink of a header rather than the header's canonical location, which can create confusion as VSCode treats them as two separate files being edited independently, then gets confused if you accidentally edit both of them.

I think we might be better served by a simpler compiler commands generator that does not attempt to exactly match what Bazel will do. Instead, it should manually set up -I flags pointing at the canonical locations of each dependency. Yes, this means that if you #include a header without adding the right dependency in the BUILD file, VSCode won't flag the error, and you won't find out until you try to run Bazel and it complains. I think that's actually a feature, not a bug -- I would prefer not to have to interrupt my coding thought process to go fix BUILD files. It's fine to have that flagged later and then fix it then.

ohodson added a commit that referenced this issue Jun 20, 2023
A step towards retiring the problematic compile_commands.json.

WIP: part macOS, part Linux, no part Windows.

Bug: #506
ohodson added a commit that referenced this issue Jun 20, 2023
A step towards retiring the problematic compile_commands.json.

WIP: part macOS, part Linux, no part Windows.

Bug: #506
ohodson added a commit that referenced this issue Jun 21, 2023
A step towards retiring the problematic compile_commands.json.

WIP: part macOS, part Linux, no part Windows.

Bug: #506
ohodson added a commit that referenced this issue Jun 21, 2023
A step towards retiring the problematic compile_commands.json.

WIP: part macOS, part Linux, no part Windows.

Bug: #506
ohodson added a commit that referenced this issue Jun 25, 2023
This change moves to using compile_flags.txt with clangd rather than
compile_commands.json.

o Adds compile_flags.txt.
o Drops support for generating compile_commands.json with Bazel.
o Add script to create the ${workspaceFolder}/external/ symlink that
  compile_flags.txt uses.

Bug: #506
ohodson added a commit that referenced this issue Jun 25, 2023
This change moves to using compile_flags.txt with clangd rather than
compile_commands.json.

o Adds compile_flags.txt.
o Drops support for generating compile_commands.json with Bazel.
o Add script to create the ${workspaceFolder}/external/ symlink that
  compile_flags.txt uses.

Bug: #506
ohodson added a commit that referenced this issue Jun 26, 2023
This change moves to using compile_flags.txt with clangd rather than
compile_commands.json.

o Adds compile_flags.txt.
o Drops support for generating compile_commands.json with Bazel.
o Add script to create the ${workspaceFolder}/external/ symlink that
  compile_flags.txt uses.

Bug: #506
ohodson added a commit that referenced this issue Jun 26, 2023
This change moves to using compile_flags.txt with clangd rather than
compile_commands.json.

o Adds compile_flags.txt.
o Drops support for generating compile_commands.json with Bazel.
o Add script to create the ${workspaceFolder}/external/ symlink that
  compile_flags.txt uses.

Bug: #506
ohodson added a commit that referenced this issue Jun 26, 2023
ohodson added a commit that referenced this issue Jun 26, 2023
This change moves to using compile_flags.txt with clangd rather than
compile_commands.json.

o Adds compile_flags.txt.
o Drops support for generating compile_commands.json with Bazel.
o Add script to create the ${workspaceFolder}/external/ symlink that
  compile_flags.txt uses.

Bug: #506
ohodson added a commit that referenced this issue Jun 26, 2023
ohodson added a commit that referenced this issue Jun 26, 2023
This change moves to using compile_flags.txt with clangd rather than
compile_commands.json.

o Adds compile_flags.txt.
o Drops support for generating compile_commands.json with Bazel.
o Add script to create the ${workspaceFolder}/external/ symlink that
  compile_flags.txt uses.

Bug: #506
ohodson added a commit that referenced this issue Jun 26, 2023
ohodson added a commit that referenced this issue Jun 27, 2023
Minor #include re-orderings for clangd considering headers as
standalone files.

Bug: #506
ohodson added a commit that referenced this issue Jun 27, 2023
This change moves to using compile_flags.txt with clangd rather than
compile_commands.json.

o Adds compile_flags.txt.
o Drops support for generating compile_commands.json with Bazel.
o Add script to create the ${workspaceFolder}/external/ symlink that
  compile_flags.txt uses.

Bug: #506
ohodson added a commit that referenced this issue Jun 27, 2023
Minor #include re-orderings for clangd considering headers as
standalone files.

Bug: #506
@ohodson
Copy link
Contributor Author

ohodson commented Sep 21, 2023

Replaced use of compile_commands.json with pre-canned compile_flags.txt. When compile_flags.txt gets stale, there's a bash script, workerd/tools/unix/clangd-check.sh, intended to help fix issues (workerd advocates using clangd for language services).

@ohodson ohodson closed this as completed Sep 27, 2023
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

No branches or pull requests

2 participants