fix: accumulate protobuf descriptors #192
fix: accumulate protobuf descriptors #192ennru merged 2 commits intolightbend:mainfrom ennru:ennru-eventing-sample-ci
Conversation
|
Currently reproducing it with the very same .proto files in a test. This way we can directly test the generation itself. |
ennru
changed the title
raboof
left a comment
raboof
left a comment
There was a problem hiding this comment.
Some questions, but if I'm reading this right it LGTM
The java-eventing-shopping-cart exercises this scenario, right?
| Using[FileInputStream, Either[CannotOpen, Iterable[ | ||
| Either[ReadFailure, Descriptors.FileDescriptor] | ||
| ]]]( | ||
| ): Either[CannotOpen, Either[ReadFailure, Iterable[Descriptors.FileDescriptor]]] = |
There was a problem hiding this comment.
So now if we fail to read one file we throw away all results? That might be OK if we're sure we need all of them anyway
There was a problem hiding this comment.
We bail out on any error now, before it could live with certain descriptors failing.
| case Left(_) => reads | ||
| case Right(fileDescriptors) => | ||
| try { | ||
| Right(fileDescriptors ++ List(Descriptors.FileDescriptor.buildFrom(file, fileDescriptors.toArray, true))) |
There was a problem hiding this comment.
So we have to pass 'depended-upon' descriptors when parsing 'depending' descriptors - meaning the files must be accumulated in the 'right order'. How do we make sure they are in the right order?
There was a problem hiding this comment.
I'm not sure they have to be in the right order, but maybe we should allowUnknownDependencies=false instead?
There was a problem hiding this comment.
I'm not sure they have to be in the right order
Whatever descriptor is the first one will be called with an empty fileDescriptors array, so if some of them need things to be available in there then that'd mean the order matters?
I'm not sure they have to be in the right order, but maybe we should
allowUnknownDependencies=falseinstead?
I'm not sure, I guess you could have unknown dependencies (like external proto annotations that we don't care about) that we could allow?
There was a problem hiding this comment.
This comment is what I expected, but the API doesn't seem to allow us to first accumulate the descriptors and after that trigger linking.
I assume allowUnknownDependencies=false would make it fail when the order is wrong, but now the placeholders will be linked once the correct descriptor has been added to the list.
ennru
left a comment
ennru
left a comment
There was a problem hiding this comment.
Yes, the initially failing builds show how it handled the descriptors in isolation and "invented" placeholders for unresolved references.
| case Left(_) => reads | ||
| case Right(fileDescriptors) => | ||
| try { | ||
| Right(fileDescriptors ++ List(Descriptors.FileDescriptor.buildFrom(file, fileDescriptors.toArray, true))) |
There was a problem hiding this comment.
I'm not sure they have to be in the right order, but maybe we should allowUnknownDependencies=false instead?
| Using[FileInputStream, Either[CannotOpen, Iterable[ | ||
| Either[ReadFailure, Descriptors.FileDescriptor] | ||
| ]]]( | ||
| ): Either[CannotOpen, Either[ReadFailure, Iterable[Descriptors.FileDescriptor]]] = |
There was a problem hiding this comment.
We bail out on any error now, before it could live with certain descriptors failing.
|
Rebased as preparation to merge as two commits. |
4 participants
The Eventing Shopping Cart sample doesn't work with the current codegen AFAICS.