mbt(bazel): add a streamed_proto decoder for bazel query output - #8703
mbt(bazel): add a streamed_proto decoder for bazel query output#8703maksymilianrozanski wants to merge 1 commit into
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds Bazel streamed-proto query support, protobuf decoding, rule and target models, Scala-version source analysis, dependency extraction, fixture regeneration, and unit/integration tests. ChangesBazel streamed proto importer
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant BazelQuery
participant Bazel
participant BazelStreamedProto
participant BazelTargetsProtoDump
BazelQuery->>Bazel: Execute streamed_proto query
Bazel-->>BazelQuery: Return target byte stream
BazelQuery->>BazelStreamedProto: Parse target stream
BazelStreamedProto-->>BazelQuery: Return BazelRule map
BazelQuery->>BazelTargetsProtoDump: Build target metadata
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
bin/regenerate-bazel-proto-fixture.sh (1)
30-42: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFixture flags don't fully match production flags.
The comment says these are "the importer's production query flags," but
--proto:locations=false(line 39) isn't part ofOutputMode.StreamedProto.targetStreamArgsinBazelQuery.scala. Harmless for decoding (location fields aren't consumed), but the fixture then isn't byte-identical to real production output, and the in-script claim is misleading for future maintainers regenerating it.Align comment or flags
-# The importer's production query flags (see BazelQuery.scala). `--keep_going` +# The importer's production query flags (see BazelQuery.scala) plus +# `--proto:locations=false` to keep the fixture compact. `--keep_going` # exits 3 on partial success, which is expected here, so don't let `set -e` trip🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bin/regenerate-bazel-proto-fixture.sh` around lines 30 - 42, Align the fixture query with production by removing the extra --proto:locations=false flag from run. Update the adjacent comment so it accurately describes the shared production flags, while preserving the existing streamed-proto output and --keep_going behavior.metals/src/main/scala/scala/meta/internal/metals/mbt/importer/BazelBuildSrcs.scala (1)
31-37: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor naming ambiguity:
labelhere is a source-file label, not aselect()branch label.
label <- labelsshadows the more common use of "label" elsewhere in this codebase (BazelSelectBranch.label, theconfig_settingguard). Renaming tosrcLabel/fileLabelwould reduce confusion for future readers.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@metals/src/main/scala/scala/meta/internal/metals/mbt/importer/BazelBuildSrcs.scala` around lines 31 - 37, Rename the iteration variable `label` in the `byTarget`/`byVersion` candidate-building loop to `srcLabel` or `fileLabel`, and update its references in the `active.contains` check and `candidatesByLabel` lookup/update; preserve the existing behavior.metals/src/main/scala/scala/meta/internal/metals/mbt/importer/BazelScalaVersions.scala (1)
18-22: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider logging unparseable versions as warnings rather than errors.
Since arbitrary string values (e.g.,
"latest") can be legitimately present in Bazel configurations and are simply skipped by this logic without breaking the importer, anerrorlog might be too loud and alert users unnecessarily. Awarnlog might be more appropriate.♻️ Proposed fix
- case Failure(_) => - scribe.error( - s"bazel-mbt: could not parse Scala version '$version'; ignoring it" - ) - None + case Failure(_) => + scribe.warn( + s"bazel-mbt: could not parse Scala version '$version'; ignoring it" + ) + None🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@metals/src/main/scala/scala/meta/internal/metals/mbt/importer/BazelScalaVersions.scala` around lines 18 - 22, In the Failure branch handling unparseable Scala versions, update the scribe logging call from error-level to warning-level while preserving the existing message and None return behavior.tests/slow/src/test/scala/tests/bazel/BazelStreamedProtoBazel5QuerySuite.scala (1)
68-70: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
NonFatalinstead ofExceptionfor safer error catching.Catching
Exceptioncan inadvertently swallow control-flow exceptions (likeInterruptedException). In Scala, it is considered best practice to usescala.util.control.NonFatalfor capturing general errors.♻️ Proposed fix
- } catch { - case e: Exception => + } catch { + case scala.util.control.NonFatal(e) =>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/slow/src/test/scala/tests/bazel/BazelStreamedProtoBazel5QuerySuite.scala` around lines 68 - 70, Update the shutdown error handler in BazelStreamedProtoBazel5QuerySuite to match NonFatal rather than catching Exception, importing scala.util.control.NonFatal if needed. Preserve the existing warning message and shutdown handling for non-fatal failures.metals/src/main/scala/scala/meta/internal/metals/mbt/importer/BazelTargetsProtoDump.scala (1)
93-102: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winOptimize BFS to mark nodes as seen upon enqueuing.
In the current implementation, nodes are marked as seen only after they are dequeued. If multiple paths lead to the same unvisited target in the dependency graph, it will be enqueued multiple times, which can cause the queue to grow exponentially in dense graphs. Marking nodes as seen when enqueuing prevents duplicate processing and queue bloat.
⚡ Proposed fix for queue optimization
- val seen = scala.collection.mutable.LinkedHashSet.empty[String] - val queue = scala.collection.mutable.Queue(root) - while (queue.nonEmpty) { - val current = queue.dequeue() - if (!seen(current)) { - seen += current - for (dep <- adjacency.getOrElse(current, Nil)) { - if (!seen(dep)) queue.enqueue(dep) - } - } - } + val seen = scala.collection.mutable.LinkedHashSet.empty[String] + seen += root + val queue = scala.collection.mutable.Queue(root) + while (queue.nonEmpty) { + val current = queue.dequeue() + for (dep <- adjacency.getOrElse(current, Nil)) { + if (!seen(dep)) { + seen += dep + queue.enqueue(dep) + } + } + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@metals/src/main/scala/scala/meta/internal/metals/mbt/importer/BazelTargetsProtoDump.scala` around lines 93 - 102, Update the BFS loop around the seen set and queue so nodes are marked as seen when they are enqueued, including root initialization and newly discovered dependencies. Enqueue each dependency only when it is newly added to seen, and remove the redundant dequeue-time seen guard while preserving traversal order.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@jsemanticdb/src/main/protobuf/bazel_query.proto`:
- Around line 8-11: Update the protobuf package declaration in BazelQueryProto
to match the file’s directory structure and satisfy buf’s
PACKAGE_DIRECTORY_MATCH rule, while preserving the existing Java package and
outer class options.
---
Nitpick comments:
In `@bin/regenerate-bazel-proto-fixture.sh`:
- Around line 30-42: Align the fixture query with production by removing the
extra --proto:locations=false flag from run. Update the adjacent comment so it
accurately describes the shared production flags, while preserving the existing
streamed-proto output and --keep_going behavior.
In
`@metals/src/main/scala/scala/meta/internal/metals/mbt/importer/BazelBuildSrcs.scala`:
- Around line 31-37: Rename the iteration variable `label` in the
`byTarget`/`byVersion` candidate-building loop to `srcLabel` or `fileLabel`, and
update its references in the `active.contains` check and `candidatesByLabel`
lookup/update; preserve the existing behavior.
In
`@metals/src/main/scala/scala/meta/internal/metals/mbt/importer/BazelScalaVersions.scala`:
- Around line 18-22: In the Failure branch handling unparseable Scala versions,
update the scribe logging call from error-level to warning-level while
preserving the existing message and None return behavior.
In
`@metals/src/main/scala/scala/meta/internal/metals/mbt/importer/BazelTargetsProtoDump.scala`:
- Around line 93-102: Update the BFS loop around the seen set and queue so nodes
are marked as seen when they are enqueued, including root initialization and
newly discovered dependencies. Enqueue each dependency only when it is newly
added to seen, and remove the redundant dequeue-time seen guard while preserving
traversal order.
In
`@tests/slow/src/test/scala/tests/bazel/BazelStreamedProtoBazel5QuerySuite.scala`:
- Around line 68-70: Update the shutdown error handler in
BazelStreamedProtoBazel5QuerySuite to match NonFatal rather than catching
Exception, importing scala.util.control.NonFatal if needed. Preserve the
existing warning message and shutdown handling for non-fatal failures.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 90281adb-6207-44ed-88f1-52979f2c32b9
📒 Files selected for processing (15)
bin/regenerate-bazel-proto-fixture.shjsemanticdb/src/main/protobuf/bazel_query.protometals/src/main/scala/scala/meta/internal/builds/BazelBuildTool.scalametals/src/main/scala/scala/meta/internal/metals/mbt/importer/BazelBuildSrcs.scalametals/src/main/scala/scala/meta/internal/metals/mbt/importer/BazelQuery.scalametals/src/main/scala/scala/meta/internal/metals/mbt/importer/BazelRule.scalametals/src/main/scala/scala/meta/internal/metals/mbt/importer/BazelScalaVersions.scalametals/src/main/scala/scala/meta/internal/metals/mbt/importer/BazelStreamedProto.scalametals/src/main/scala/scala/meta/internal/metals/mbt/importer/BazelTargetsProtoDump.scalatests/slow/src/test/scala/tests/bazel/BazelStreamedProtoBazel5QuerySuite.scalatests/unit/src/test/resources/bazel/rules-scala-fullinfo-fragments.pbtests/unit/src/test/scala/tests/bazel/BazelScalaVersionSuite.scalatests/unit/src/test/scala/tests/bazel/BazelStreamedProtoSuite.scalatests/unit/src/test/scala/tests/bazel/BazelTargetsProtoDumpRealOutputSuite.scalatests/unit/src/test/scala/tests/bazel/BazelUnconfiguredSourcesSuite.scala
9c3e37f to
13c20ec
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
|
||
| import scala.meta.internal.metals.mbt.importer.BazelBuildSrcs.TargetSrcs | ||
|
|
||
| class BazelTargetsProtoDump(rulesByName: Map[String, BazelRule]) { |
There was a problem hiding this comment.
Based on BazelTargetsXmlDump.
BazelTargetsXmlDump is going to be removed after the migration.
A parsing layer for `bazel query --output=streamed_proto --proto:flatten_selects=false`, not yet used by the importer: - bazel_query.proto (jsemanticdb): a trimmed subset of Bazel's build.proto — field numbers identical, unknown fields skipped — compiled by the existing protoc setup. - BazelStreamedProto: decodes the length-delimited binary Target stream into BazelRule values whose attributes keep select() branches keyed by their config_setting label (BazelSelectBranch). - BazelTargetsProtoDump: the per-target views the importer reads (srcs with filegroup expansion, rule classes/outputs/deps, import-target jars), mirroring BazelTargetsXmlDump. - BazelBuildSrcs: partitions a rule's srcs by scala_version select() branch and recovers inactive-branch sources with their version + origin target. - BazelScalaVersions.maxVersion: SemVer-max over free-form version strings. - BazelQuery: OutputMode.StreamedProto + runProtoDump, capturing stdout as raw bytes (line-based capture would mangle the binary stream). The XML and cquery paths are untouched; fullInformationQuery still runs XML. Covered by unit suites over synthetic wire bytes (BazelStreamedProtoSuite), captured real Bazel 7.7.1 output (BazelTargetsProtoDumpRealOutputSuite, .pb fixture regenerated by bin/regenerate-bazel-proto-fixture.sh), inactive-source recovery (BazelUnconfiguredSourcesSuite), and a slow suite running the real query against Bazel 5.0.0 — Metals' minimum supported version, which rejects streamed_jsonproto (BazelStreamedProtoBazel5QuerySuite).
7a4ccae to
25d3166
Compare
| @@ -0,0 +1,51 @@ | |||
| #!/usr/bin/env bash | |||
There was a problem hiding this comment.
MAybe it makes sense to put it into test resources?
tgodzik
left a comment
tgodzik
left a comment
There was a problem hiding this comment.
Finally managed to review it. @maksymilianrozanski do you still have time to take a look at it?
|
|
||
| // Highest SemVer-parseable version; `scala_version` is a free-form STRING, so | ||
| // unparseable values are logged and skipped. | ||
| def maxVersion(versions: Iterable[String]): Option[String] = |
There was a problem hiding this comment.
We can probable move it to ScalaVersions class, which handles this already.
| case class TargetSrcs( | ||
| unconditional: Set[String], | ||
| byVersion: Map[String, Set[String]], | ||
| ) { | ||
| def activeFor(scalaVersion: Option[String]): Set[String] = | ||
| unconditional ++ scalaVersion.flatMap(byVersion.get).getOrElse(Set.empty) | ||
| } |
There was a problem hiding this comment.
| case class TargetSrcs( | |
| unconditional: Set[String], | |
| byVersion: Map[String, Set[String]], | |
| ) { | |
| def activeFor(scalaVersion: Option[String]): Set[String] = | |
| unconditional ++ scalaVersion.flatMap(byVersion.get).getOrElse(Set.empty) | |
| } | |
| case class TargetSrcs( | |
| alwaysIncludedFiles: Set[String], | |
| versionSpecificFiles: Map[String, Set[String]], | |
| ) { | |
| def activeFilesForVersion(scalaVersion: Option[String]): Set[String] = | |
| alwaysIncluded ++ scalaVersion.flatMap(versionSpecificFiles.get).getOrElse(Set.empty) | |
| } |
it took me a while to understand, so this is maybe a bit better?
|
|
||
| case class InactiveSource(version: String, originTarget: String) | ||
|
|
||
| // Highest inactive-branch version wins; ties broken by smallest origin label. |
There was a problem hiding this comment.
Maybe add a bit more why use it, why do we have inactive sources in bazel.
| Files.writeString(workspace.resolve(name).toNIO, content) | ||
|
|
||
| override def beforeAll(): Unit = { | ||
| write(".bazelversion", s"$bazelVersion\n") |
There was a problem hiding this comment.
I would use FileLayout for the tests here
|
|
||
| import tests.BaseSuite | ||
|
|
||
| class BazelScalaVersionSuite extends BaseSuite { |
There was a problem hiding this comment.
We can have this in ScalaVersionsSuite
|
|
||
| // [[BazelTargetsProtoDump]] over synthetic wire bytes built with Bazel's | ||
| // `build.proto` field numbers, including fields the decoder must skip. | ||
| class BazelStreamedProtoSuite extends BaseSuite { |
There was a problem hiding this comment.
Why not use the same approach like in BazelStreamedProtoBazel5QuerySuite of just running Bazel. The code here seems tricky to understand.
There was a problem hiding this comment.
MAybe we don't need this suite at all? The next suite seems to test it much better.
There was a problem hiding this comment.
Yes, we could achieve a similar test coverage with
BazelStreamedProtoBazel5QuerySuite.
One area which is tested in BazelStreamedProtoSuite and not tested in BazelStreamedProtoBazel5QuerySuite is jarLabelsByImportTarget and sourceJarByImportTarget fields. Aside from this one difference, I agree that we could remove BazelStreamedProtoSuite.
Thanks for the review! I won't be able to rebase and re-test right now, feel free to push to my branch or raise a fresh PR. |
2 participants
A parsing layer for
bazel query --output=streamed_proto --proto:flatten_selects=false.part of #8494
No observable changes yet.
Summary by CodeRabbit