Skip to content

Fix Protobuf header resolution with import_path, strip_import_path - #215

Merged
jayconrod merged 3 commits into
EngFlow:mainfrom
gg-sr:gg-sr/fix-protobuf-path-resolution
May 1, 2026
Merged

Fix Protobuf header resolution with import_path, strip_import_path#215
jayconrod merged 3 commits into
EngFlow:mainfrom
gg-sr:gg-sr/fix-protobuf-path-resolution

Conversation

@gg-sr

@gg-sr gg-sr commented Apr 27, 2026

Copy link
Copy Markdown
Contributor

This relies on translating the include paths from foo/bar.pb.h to foo/bar.proto and then using the proto import resolution. I originally tried to implement the whole logic by parsing gazelle:proto_{strip_,}import_path and applying it when we generate rules, but it was more complex, more fragile, and didn't work in all the cases tested in this commit.

Signed-off-by: Grégoire Geis <gregoire@seoulrobotics.org>
@gg-sr
gg-sr force-pushed the gg-sr/fix-protobuf-path-resolution branch from fe95852 to 6074ddf Compare April 27, 2026 05:33
@jayconrod

Copy link
Copy Markdown
Collaborator

Could you explain the bug being fixed here? I'm not sure I understand.

@gg-sr

gg-sr commented Apr 29, 2026

Copy link
Copy Markdown
Contributor Author

gazelle_cc supports gazelle:cc_strip_include_prefix and gazelle:cc_include_prefix for compatibility with projects that have "non-Bazel-standard" layout.

gazelle similarly has gazelle:proto_strip_import_prefix and gazelle:proto_import_prefix, which work similarly, but for import "..." statements in Protobuf.

Before this PR, if gazelle_cc encountered #include "some/path.pb.h", it would only try to resolve some/path.pb.h, which would hit whatever library was generated by getGeneratedFilesFromProtoPackage() in proto.go, which always uses the file name + .pb.h.

However, if strip_import_prefix or import_prefix is set in the proto_library, then the cc_proto_library will also adjust its include path, similarly to if strip_include_prefix or include_prefix had been set. So //foo/bar/baz.proto needs to be included as bar/baz.pb.h if its strip_include_prefix is /foo.

This PR relies on the proto gazelle extension to support that resolution. Ideally we would directly read from import_prefix or strip_import_prefix in generateProtoLibraryRules() to generate the proper paths, but these aren't exposed by proto.Package. We could also parse the gazelle:proto_*_prefix directives ourselves, which was my first approach, but it's more work and more error prone.

As an example, without the fix the new test will fail to resolve the Protobuf files:

gazelle: //dir:main: could not find a library providing header - '#include "proto/dir/with_import_prefix/b.pb.h"' at dir/main.cc:2
gazelle: //dir:main: could not find a library providing header - '#include "proto/subpackage/with_import_prefix/d.pb.h"' at dir/main.cc:3
gazelle: //dir:main: could not find a library providing header - '#include "subpackage/c.pb.h"' at dir/main.cc:4
gazelle: //stripped_abs_prefix/subpackage:main: could not find a library providing header - '#include "proto/dir/with_import_prefix/b.pb.h"' at stripped_abs_prefix/subpackage/main.cc:2
gazelle: //stripped_abs_prefix/subpackage:main: could not find a library providing header - '#include "proto/subpackage/with_import_prefix/d.pb.h"' at stripped_abs_prefix/subpackage/main.cc:3
gazelle: //stripped_abs_prefix/subpackage:main: could not find a library providing header - '#include "subpackage/c.pb.h"' at stripped_abs_prefix/subpackage/main.cc:4

jayconrod
jayconrod previously approved these changes Apr 29, 2026
@jayconrod
jayconrod requested a review from afrueda97 April 29, 2026 18:38
@jayconrod
jayconrod dismissed their stale review April 29, 2026 18:38

Test failure

@jayconrod

Copy link
Copy Markdown
Collaborator

Thanks for explaining. This looks good.

CI is broken due to a misconfiguration on my part. I recently enabled remote caching, but the secret needed to make that work is not available on PRs from external contributors. I'll fix that separately. Until that's fixed, I'm fine with running the tests manually to verify.

But on my machine bazel test //... fails with this message:

$ bazel test //...
INFO: Invocation ID: 541ea46f-bc5e-4141-a47b-19acfc3bae67
ERROR: no such package '@@[unknown repo 'compilation_test_protobuf_import_prefix' requested from @@]//': The repository '@@[unknown repo 'compilation_test_protobuf_import_prefix' requested from @@]' could not be resolved: No repository visible as '@compilation_test_protobuf_import_prefix' from main repository
ERROR: no such package '@@[unknown repo 'compilation_test_protobuf_import_prefix' requested from @@]//': The repository '@@[unknown repo 'compilation_test_protobuf_import_prefix' requested from @@]' could not be resolved: No repository visible as '@compilation_test_protobuf_import_prefix' from main repository
INFO: Elapsed time: 0.087s
INFO: 0 processes.
ERROR: Build did NOT complete successfully
ERROR: Couldn't start the build. Unable to run tests

Please fix that first.

gg-sr added 2 commits April 30, 2026 07:29
Signed-off-by: Grégoire Geis <gregoire@seoulrobotics.org>
Signed-off-by: Grégoire Geis <gregoire@seoulrobotics.org>
@gg-sr

gg-sr commented Apr 29, 2026

Copy link
Copy Markdown
Contributor Author

Ah right, my bad, I forgot to update MODULE.bazel (fixed in c2331b2).

I also changed the tests slightly to use both gazelle:proto default and gazelle:proto file, just to vary the behavior a little bit (b065ca5).

@jayconrod
jayconrod merged commit bcf4957 into EngFlow:main May 1, 2026
1 check passed
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