desc/builder: custom options aren't properly resolved and are missing from built files' imports about protoreflect HOT 9 CLOSED

I'm guessing this issue might apply to all other option types (MethodOptions, FieldOptions, etc.) as well then?

Yep. I'm looking at fixing it now: there are some tricks to interacting with options -- mainly if a descriptor has custom options that are not compiled into the Go program, in which case an extension registry is required. I think that will require some new API, to set an extension registry on a builder (or perhaps to supply one during the call to Build).

from protoreflect.

@noahdietz, I just created a 1.2.0 release. There were numerous other fixes and additions on master since 1.1.0, so it seemed about time to go ahead and cut a new version.

from protoreflect.

I could be totally misunderstanding things, but I changed the protoprint tests to have the same custom option injected into the Service (if one was present) using the same code above (proto.SetExtension on the reference provided by GetServiceOptions() if present). I assumed the test actual output would differ from the golden as a result and fail the tests. This did not happen. Granted, this is a slightly different scenario, using a parsed protoset (tests) rather than one built via the builder pkg (my code). Lmk if I'm off-base or doing something wrong here. Thanks

EDIT: forgot to mention that when I included the aforementioned fix (including dm.GetKnownExtensions), the tests failed in the way I expected, the custom option was included in the test actual output.

from protoreflect.

@noahdietz, the problem is actually in the desc/builder package: when you build a descriptor for the service, it is failing to add that option's file to the imports of the service's enclosing file. So, since the file does not import that option, the printer has no knowledge of it.

from protoreflect.

Ah ok thanks @jhump for looking into and clarifying the issue. I'm guessing this issue might apply to all other option types (MethodOptions, FieldOptions, etc.) as well then?

from protoreflect.

@noahdietz, if you are able to give this a whirl, fetch latest and checkout the branch named
jh/builders-should-add-deps-for-options.

I think that will fix the issue you are encountering. In the meantime, I need to write some add'l test cases for this.

from protoreflect.

Worked like a charm!

Really appreciate the quick turn around.

from protoreflect.

@jhump out of curiosity, do you plan on making a release soon? I'm pinned to 1.1.0 currently, just wondering what to expect. Thanks

from protoreflect.

Thank you! Much appreciated

from protoreflect.