raboof commented on PR #746: URL: https://github.com/apache/pekko-grpc/pull/746#issuecomment-4788435754
> 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_ generate the classes 😆 . 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 classes not being generated 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. -- 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]
