He-Pin commented on PR #746:
URL: https://github.com/apache/pekko-grpc/pull/746#issuecomment-4788257012
## Code Review
### š“ Removing `PB.recompile` config block likely breaks proto class
generation
The removed `inConfig(config)(Seq(...))` block (lines 139ā161) did more than
manage resource directories ā it also configured `PB.recompile / sources` and
`PB.recompile / unmanagedSources`, which control **which proto files are
discovered for compilation**. Without this explicit configuration, sbt-protoc's
defaults may not discover the project's own proto files from `sourceDirectory /
"proto"`, which likely explains the observed behavior of classes not being
generated with `google-cloud-pub-sub-grpc`.
The `PB.recompile / unmanagedSources` and `PB.recompile / sources` settings
are the ones that drive protoc's input discovery. The resource-related settings
(`PB.recompile / resources`, `PB.recompile / unmanagedResources`) were only
part of what that block did.
**Suggestion**: Keep the source-related settings (`PB.recompile /
includeFilter`, `PB.recompile / unmanagedSourceDirectories`, `PB.recompile /
sources`) and only remove the resource-related ones that are being replaced by
the new `packageBin / mappings` approach. Or verify that sbt-protoc's defaults
already cover this.
---
### š” `withoutDuplicates` has O(nĀ²) complexity
In `PekkoGrpcPlugin.scala`, the tail-recursive `withoutDuplicates` uses
`soFar :+ ((file, string))` which is O(n) per append on a `Seq`, making the
overall loop O(nĀ²). For a small number of mappings this is fine, but it's easy
to make it O(n):
```scala
@scala.annotation.tailrec
def withoutDuplicates(toAdd: Seq[(File, String)], seen: Set[String], acc:
List[(File, String)])
: Seq[(File, String)] = {
toAdd.headOption match {
case Some((file, path)) =>
if (seen.contains(path)) withoutDuplicates(toAdd.tail, seen, acc)
else withoutDuplicates(toAdd.tail, seen + path, (file, path) :: acc)
case None => acc.reverse
}
}
withoutDuplicates(mappingsToAdd, existingMappings.map(_._2).toSet,
existingMappings.toList)
```
---
### š” Parameter name `string` is unclear
In `withoutDuplicates`, the parameter name `string` for the second tuple
element is not descriptive. `path` or `relativePath` would better convey that
it's the target path within the JAR.
---
### š” Scripted test assertion is fragile
In `build.sbt`:
```scala
assert(!(Compile /
unmanagedResourceDirectories).value.mkString.contains("target/protobuf_external_src"))
assert((Compile /
managedResourceDirectories).value.mkString.contains("target/protobuf_external_src"))
```
Using `.mkString.contains(...)` on a `Seq[File]` is fragile ā it could match
partial path segments or produce different results depending on path
separators. A more robust approach:
```scala
val unmanagedDirs = (Compile / unmanagedResourceDirectories).value
assert(!unmanagedDirs.exists(_.getPath.endsWith("protobuf_external_src")),
s"protobuf_external_src should not be in unmanagedResourceDirectories:
$unmanagedDirs")
val managedDirs = (Compile / managedResourceDirectories).value
assert(managedDirs.exists(_.getPath.endsWith("protobuf_external_src")),
s"protobuf_external_src should be in managedResourceDirectories:
$managedDirs")
```
This gives clearer failure messages and avoids false positives from
substring matching.
---
### Summary
The core idea ā moving proto files from unmanaged to managed classpath and
using `packageBin / mappings` for explicit control ā makes sense. The main
concern is that removing the entire `PB.recompile` configuration block removes
source discovery settings alongside the resource settings, which is likely the
root cause of the broken class generation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
