bazel-contrib / bazel-gazelle

Gazelle is a Bazel build file generator for Bazel projects. It natively supports Go and protobuf, and it may be extended to support new languages and custom rule sets.
Apache License 2.0
1.19k stars 378 forks source link

Incorrect `proto_library` `deps` when `strip_import_prefix`, `--index=false` and `mode=file` #1934

Open nchepanov opened 3 weeks ago

nchepanov commented 3 weeks ago

What version of gazelle are you using?

Latest, v.0.39.0

What version of rules_go are you using? / What version of Bazel are you using? / Does this issue reproduce with the latest releases of all the above?

N.A.

What operating system and processor architecture are you using?

Darwin nchepanov-mac 23.6.0 Darwin Kernel Version 23.6.0: Mon Jul 29 21:14:30 PDT 2024; root:xnu-10063.141.2~1/RELEASE_ARM64_T6000 arm64 MacOS 14.6.1

What did you do?

Reproducer https://github.com/nchepanov/gazelle-strip-import-prefix-index-off/

In our repository all protos live in a special directory within the monorepo. For historical reasons they reference each other relative to the directory, not the repository root. We are trying to speed up gazelle by using --index=false but it seems to break the proto_strip_import_prefix directive rewriting of the imports during language/proto/resolve.go:resolveProto()

In essence, --index=false + strip_import_prefix results in dropping the //prefix in deps that was there with indexing on

  diff --git a/prefix/foo/BUILD.bazel b/prefix/foo/BUILD.bazel
  index fab6b89..037722e 100644
  --- a/prefix/foo/BUILD.bazel
  +++ b/prefix/foo/BUILD.bazel
  @@ -5,5 +5,5 @@ proto_library(
      srcs = ["foo.proto"],
      strip_import_prefix = "/prefix",
      visibility = ["//visibility:public"],
  -    deps = ["//prefix/moo:moo_proto"],
  +    deps = ["//moo:moo_proto"],
  )

What did you expect to see?

The strip_import_prefix attribute is generated in both situations, so the root BUILD.bazel file was read with or without indexing. The deps attribute should be the same in both cases.

nchepanov commented 3 weeks ago

I'm working on a fix, essentially taking this code here that's only activated when building an index

https://github.com/bazelbuild/bazel-gazelle/blob/d16fc424cfcfdbd8893a267e500ec5d04ec0bbf0/language/proto/resolve.go#L40-L66

extracting it as a function and adding it to here (conditional on whether indexing was enabled)

https://github.com/bazelbuild/bazel-gazelle/blob/d16fc424cfcfdbd8893a267e500ec5d04ec0bbf0/language/proto/resolve.go#L138

nchepanov commented 2 weeks ago

UPD: In a general case, such mapping is impossible without indexing, however in our case with mode=file, index=false, strip_import_prefix=set the tail end of the resolveProto() function can actually use this knowledge to map import to deps without index: https://github.com/bazelbuild/bazel-gazelle/blob/d16fc424cfcfdbd8893a267e500ec5d04ec0bbf0/language/proto/resolve.go#L145-L152

The following work in progress PR (please ignore changes to resolve.go) https://github.com/nchepanov/bazel-gazelle/pull/1 contains a fully functional test that fails as expected, showcasing the unsupported edge-case.

For now, I don't have more time to dig more into this, I ended up solving this in our custom language by using CrossResolve that was introduced in https://github.com/bazelbuild/bazel-gazelle/issues/771 to support a very similar use-case: full indexing is too slow.

For reference, a super simple CrossResolve impelmentation ended up looking like this

func (l *customLang) CrossResolve(c *config.Config, ix *resolve.RuleIndex, imp resolve.ImportSpec, lang string) []resolve.FindResult {
    if lang == "proto" && imp.Lang == "proto" {
        rel := path.Dir(imp.Imp)
        if rel == "." {
            rel = ""
        }
        filename := strings.TrimSuffix(path.Base(imp.Imp), ".proto")
        return []resolve.FindResult{
            {Label: label.New("", path.Join("prefix", rel), proto.RuleName(filename))},
        }
    }
    return nil
}