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

Support recently added "source_file_descriptors" field of CodeGeneratorRequest #2792

Merged
merged 11 commits into from
Mar 7, 2024

Conversation

jhump
Copy link
Member

@jhump jhump commented Feb 28, 2024

I've also made sure this branch can apply to bufmod. Here's my tentative cherry-pick:
bufmod...jh/strip-source-only-on-bufmod

@jhump jhump force-pushed the jh/strip-source-only-options branch from 6d11262 to f1dbb1c Compare February 29, 2024 14:42
@jhump jhump requested a review from bufdev February 29, 2024 14:42
…de source-only options in separate source_file_descriptors field, only for files named in file_to_generate
@jhump jhump force-pushed the jh/strip-source-only-options branch from f1dbb1c to 0d2c5bc Compare February 29, 2024 14:44
Copy link
Member

@bufdev bufdev left a comment

Choose a reason for hiding this comment

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

An overall thing: I think we might want to make it so that we can select source-retention options or not at the ImageFile level. A proposal for how I'd do this:

type ImageFile interface {
  ...
  FileDescriptorProto() *descriptorpb.FileDescriptorProto
  FileDescriptorProtoWithoutSourceRetentionOptions() (*descriptorpb.FileDescriptorProto, error)
  ...
}

Then, in imageFile, I would do:

type imageFile struct {
  ...
  getFileDescriptorProtoWithoutSourceRetentionOptions func() (*descriptorpb.FileDescriptorProto, error)
}

And on construction:

func newImageFileNoValidate(...) {
  imageFile := &imageFile{...}
  // Make sure to use syncext instead of sync as OnceValues was not in Golang <1.21.
  imageFile.getFileDescriptorProtoWithoutSourceRetentionOptions =syncext.OnceValues(imageFile.getFileDescriptorProtoWithoutSourceRetentionOptionsUncached)
  return imageFile
}

And at the bottom of image_file.go, before isImageFileInfo/isImageFile:

func (f *imageFile) getFileDescriptorProtoWithoutSourceRetentionOptionsUncached() (*descriptorpb.FileDescriptorProto, error) {
  return stripSourceRetentionOptions(f.fileDescriptorProto)
}

private/bufpkg/bufimage/util.go Outdated Show resolved Hide resolved
private/bufpkg/bufimage/strip_source_only.go Outdated Show resolved Hide resolved
@jhump
Copy link
Member Author

jhump commented Mar 6, 2024

An overall thing: I think we might want to make it so that we can select source-retention options or not at the ImageFile level.

@bufdev, this approach could expose the functionality for a *descriptorpb.FileDescriptorProto. But we'll also need this functionality for *imagev1.ImageFile (so we can optionally exclude these options from the output of buf build). So, in ca03cd0, I instead opted to export StripSourceRetentionOptionsFromFile and make it generic so that it can handle either representation of a file. WDYT?

Also, I tackled the TODO for more validation of the CodeGeneratorRequest in 6e767e4. WDYT? Too much? I can easily back it out if so.

private/bufpkg/bufimage/util.go Outdated Show resolved Hide resolved
Copy link
Member

@bufdev bufdev left a comment

Choose a reason for hiding this comment

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

Thanks for getting through this! Merge into bufmod after we merge here.

@bufdev bufdev merged commit 83e33fc into main Mar 7, 2024
8 checks passed
@bufdev bufdev deleted the jh/strip-source-only-options branch March 7, 2024 19:53
jhump added a commit that referenced this pull request Mar 7, 2024
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.

2 participants