Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat!: add HybridResolver #62

Merged
merged 8 commits into from
May 3, 2023
Merged

feat!: add HybridResolver #62

merged 8 commits into from
May 3, 2023

Conversation

aaronc
Copy link
Member

@aaronc aaronc commented May 1, 2023

This provides a quick way to get accessed to a merged protodesc.Resolver by building a *protoregistry.Files for all the gogo files and then implementing a resolver on top. The merged registry functionality can then only be used when validation of conflicts is needed.

@aaronc aaronc requested a review from a team as a code owner May 1, 2023 18:14
@aaronc aaronc requested a review from mark-rushakoff May 1, 2023 18:14
message MyMessageSet {
option message_set_wire_format = true;
extensions 100 to max;
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was necessary to delete this plus all references to it because message sets are a protov1 feature unsupported by the official protobuf and so registering this file descriptor was failing

@@ -596,13 +603,52 @@ func MessageType(name string) reflect.Type {

// A registry of all linked proto files.
var (
protoFiles = make(map[string][]byte) // file name => fileDescriptor
protoFiles = make(map[string][]byte) // file name => fileDescriptor
gogoProtoRegistry = new(protoregistry.Files)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this can actually be made public like *GlobalFiles and then other checks can be performed by the caller if needed?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exported this as GogoResolver. Let me know if you see any issue with that

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it should be okay exported.

Copy link
Member

@mark-rushakoff mark-rushakoff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this approach!

proto/merge.go Outdated Show resolved Hide resolved
proto/merge.go Outdated Show resolved Hide resolved
@aaronc
Copy link
Member Author

aaronc commented May 2, 2023

@mark-rushakoff BenchmarkRepresentativeMergedFileDescriptors isn't building because of the argument type changed. I can gunzip and unmarshal all these descriptors but that will affect the benchmark time. How do you recommend the benchmark be updated to reflect the changes here?

@aaronc aaronc changed the title feat: add HybridResolver feat!: add HybridResolver May 2, 2023
@aaronc
Copy link
Member Author

aaronc commented May 2, 2023

Also this is technically a breaking change to a public API. I'm pretty sure this only being used by the SDK so it may be safe to consider the previous API as pre-release, but it wasn't tagged that way. Any suggestions on how to deal with this? I can do a rewrite to keep the previous API and add a new one, but not sure it's worth the effort. We could also retract the previous tag maybe...

@mark-rushakoff
Copy link
Member

BenchmarkRepresentativeMergedFileDescriptors has a call to b.ResetTimer() before the benchmark loop, so un-gzipping them before that call seems fine, and it is probably less effort than doing a new export from simd.

I'm okay with a tag retraction in go.mod, as the recent tag is only a couple weeks old and should only have been in use in the SDK.

@aaronc aaronc enabled auto-merge (squash) May 2, 2023 18:13
@aaronc
Copy link
Member Author

aaronc commented May 2, 2023

What do you think is the best way to approach the retraction? Delete the tag and create a new one? Or do we actually need a retract directive in the go.mod?

@aaronc
Copy link
Member Author

aaronc commented May 2, 2023

Also, really confused by the CI errors here. Seems like there's a lot of stuff failing unrelated to any changes in this PR.

@mark-rushakoff
Copy link
Member

I think just a retract directive in go.mod is fine. That might be a slight misuse of retraction, but I think it's okay given the narrow scope here -- "don't use this tag as it was a breaking API change, which we undid in the following tag".

I understand the CI error message -- we are enforcing the leading slash somewhere as an expectation against the cosmos protos we produce. But I am not immediately seeing why that just started happening on this PR.

@aaronc
Copy link
Member Author

aaronc commented May 3, 2023

I added a retract directive for 1.4.8

The test failures seem due to registration errors in the *protoregistry.Files or CheckImportPath. I fixed them by:

  • converting the registration errors to warnings
  • deleting one test case (typedeclimport) which had what I consider to be pathological imports which we shouldn't try to support

Going to move forward with merging this unless anyone sees any issues with this.

Copy link
Member

@julienrbrt julienrbrt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we get the changelog ready as well?

@aaronc aaronc merged commit 8051872 into main May 3, 2023
@aaronc aaronc deleted the aaronc/hybrid-resolver branch May 3, 2023 14:52
@aaronc
Copy link
Member Author

aaronc commented May 3, 2023

See #63 for the CHANGELOG

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants