consistently include proto's in built artifacts

[raboof]

Fixes #345

I observed the directory with the proto's was part of the unmanagedResourceDirectories. That might explain why they weren't included consistently: 'unmanaged' directories are for files that are expected to 'just exist' on disk, to there's no guard to make sure sbt reads that directory after it's been populated. This seemed to be caused by some code added in https://github.com/akka/akka-grpc/pull/149/changes which is intended to cover a fringe use case that probably doesn't really exist anymore, has this problem, and makes things complicated. This PRs simplifies things and makes sure the external proto directory is part of the managed classpath - but testing with google-cloud-pub-sub-grpc it seems to actually consistently not include the resources in the jar 😆 . More detective work needed.

This perhaps begs the question: are we sure we want to include the proto's in the artifacts? The generated API's should be fully usable just by the generated code, right?

[He-Pin]

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.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:

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:

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.

[raboof]

I observed the directory with the proto's was part of the unmanagedResourceDirectories. That might explain why they weren't included consistently: 'unmanaged' directories are for files that are expected to 'just exist' on disk, to there's no guard to make sure sbt reads that directory after it's been populated. This seemed to be caused by some code added in https://github.com/akka/akka-grpc/pull/149/changes which is intended to cover a fringe use case that probably doesn't really exist anymore, has this problem, and makes things complicated. This PRs simplifies things and makes sure the external proto directory is part of the managed classpath - but testing with google-cloud-pub-sub-grpc it seems to actually consistently not include the resources in the jar 😆 . More detective work needed.

We have to add them to the mappings instead of populating managedResourceDirectories to make them show up.

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"

If that were true shouldn't the scripted tests have caught this?

which likely explains the observed behavior of classes not being generated with google-cloud-pub-sub-grpc.

That doesn't make any sense - the behavior of proto's not being included in the jar with google-cloud-pub-sub-grpc was observed without these changes

For a small number of mappings this is fine, but it's easy to make it O(n):

I doubt projects will have so many .proto files that this would be noticable against the load of compiling those proto files, but indeed easy enough to move to front-appending to a list, done

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.

On the other hand this makes it clearer this parameter is the string-typed representation of the target path. I'll change it when other changes are needed in that area.

[He-Pin]

nice to have this

[He-Pin]

I see.

[He-Pin]

not **.proto?

[raboof]

Nope, https://github.com/sbt/io/blob/1.12.x/io/src/main/scala/sbt/io/NameFilter.scala#L367-L373

[He-Pin]

Thanks for the clarifications — you're right on the main points:

1. PB.recompile block removal: Fair point about the scripted tests — if source discovery were broken, they'd catch it. I also misread the original issue: the problem was proto resources not being included in the JAR, not class generation failure. Withdrawing the 🔴 concern.

2. O(n²): Thanks for the fix.

3. Parameter name: Fair enough, string does convey the type. Minor nit, no need to change on its own.

The approach of using packageBin / mappings for explicit control over JAR contents is the right sbt idiom — more deterministic than relying on unmanaged resource directory scanning timing.

One remaining minor note: the scripted test assertion using .mkString.contains(...) could be made more robust with .exists(_.getPath.endsWith(...)) for clearer failure messages, but that's a nit.

Overall the direction looks good. Looking forward to the final version once the google-cloud-pub-sub-grpc case is sorted out.

[He-Pin]

Why not just a mutable hashSet?

[He-Pin]

checkJarContainsProtos ?

[He-Pin]

Looks good to me. I reviewed the current head 5383e21 and found no must-fix issues.\n\nVerified locally:\n- git diff --check\n- sbt "sbt-plugin / scripted gen-scala-server/00-interop"\n- sbt "sbt-plugin / scripted gen-scala-server/03-test-config"