Add sourceinfo plugin #22

Alfus · 2024-08-07T17:13:52Z

The sourceinfo plugin provides a protoc-gen-sourceinfo cmd that generates a *.sourceinfo.binpb file, for every .proto file. Each generated file contains the associated serialized google.protobuf.SourceInfo value. These files can then be loaded directly or embedded in a binary.

The following helpers are provided for go:

sourceinfo.GlobalFiles(): A drop-in replacement for protoregistry.GlobalFiles
sourceinfo.GlobalTypes(): A drop-in replacement for protogregistry.GlobalTypes
sourceinfo.RegisterAll(root string): Registers all *.sourceinfo.binpb files found under the root dir.
sourceinfo.RegisterAllFS(fsys fs.FS, root string): Same as above, except uses the given file system.

cmd/version.go

sourceinfo.go

jhump

Just one remark about fixing up doc comments, otherwise LGTM

sourceinfo/sourceinfo.go

mfridman

Seems fine, only question is whether we want this available as a Buf-managed plugin over on https://github.com/bufbuild/plugins?

We also have https://github.com/bufbuild/tools which is a collection of public tools/plugins we use. But this plugin does appear to be closer to what this (bufbuild/protoschema-plugins) repository aims to be.

tl;dr - is this plugin something the rest of the community would want, or something we're primarily interested in? I'm not sure.

sourceinfo/sourceinfo.go

Alfus · 2024-08-07T21:34:41Z

The target use case is to enable runtime, reflection-based 'plugins'. For example, using a protobuf schema as the basis for pflags requires access to the documentation, but does not require code gen otherwise. This plugin is a building block for
any similar reflection-based used cases.

mfridman · 2024-08-07T21:40:11Z

Probably worth a mention in the README.md, even if just a brief description of what this plugin does?

internal/cmd/jsonschema-generate-testdata/main.go

internal/cmd/sourceinfo-generate-testdata/main.go

bufdev · 2024-08-08T16:16:25Z

internal/cmd/sourceinfo-generate-testdata/main.go

+		fileName := pluginsourceinfo.GetSourceInfoPath(testDesc)
+		filePath := filepath.Join(outputDir, fileName)
+		// Create any missing directories
+		if err = os.MkdirAll(filepath.Dir(filePath), 0755); err != nil {


err :=, always (this err should be scoped to this if statement)

this is a lambda that is reporting the error to the outter scope, it cannot use :=

Then it shouldn't be set inside of an if statement - that's confusing to readers. I'm actually surprised that there isn't a linter that bans this in golangci-lint

internal/cmd/sourceinfo-generate-testdata/main.go

internal/protoschema/golden/golden.go

internal/protoschema/plugin/pluginsourceinfo/pluginsourceinfo.go

bufdev · 2024-08-08T16:18:34Z

sourceinfo/sourceinfo.go

+var (
+	// GlobalFiles is a replacement for protoregistry.GlobalFiles that includes
+	// all registered source info.
+	GlobalFiles protodesc.Resolver = sourceinfo.GlobalFiles


Global variables are banned - Make this API not use global variables

This was not resolved - you just changed this from a global variable to a static function that wraps a global variable.

This is how the sourceinfo package exposes the functionality. Are you suggesting that package can't be used? Or do you have another solution in mind?

bufdev · 2024-09-03T16:07:51Z

README.md

+## SourceInfo
+
+The sourceinfo plugin provides a protoc-gen-sourceinfo command that generates an `.sourceinfo.binpb` file, for every `.proto` file.
+Each generated file contains the associated serialized `google.protobuf.SourceInfo` value.


There is no type called google.protobuf.SourceInfo. Do you mean google.protobuf.SourceCodeInfo? If so, this calls into question the naming of a lot of things here (package, plugin, etc).

Additionally, a SourceCodeInfo (if this is the intent) does not make a ton sense without an associated FileDescriptorProto - why not make the plugin provide the entire FileDescriptorProto/FileDescriptorSet?

Nice typo find! I was following @jhump's naming scheme, I am happy to switch it to source code info every where if you think that would provide more clarity.
The normal code gen already comes packed with the Descriptors for normal reflection, this plugin provides just the part that is stripped out. @jhump's sourceinfo library nicely glues everything together at runtime, they same could be done in other languages.

bufdev · 2024-09-03T16:08:11Z

README.md

+
+The following helpers are provided for go via `import "github.com/bufbuild/protoschema-plugins/sourceinfo"`:
+
+- `sourceinfo.GlobalFiles()`: A drop-in replacement for protoregistry.GlobalFiles


How can this be a drop-in if you just are serializing the google.protobuf.SourceCodeInfo file?

This is hooking into @jhump's library to provide a drop in replacement, backed by normal protoregistry values.

bufdev · 2024-09-03T16:09:54Z

internal/protoschema/plugin/pluginsourceinfo/pluginsourceinfo.go

+	return fmt.Sprintf("%s%s", path, FileExtension)
+}
+
+// GenFileContents generates the source info file contents for the given file descriptor.


I don't know what this means - what is the "source info file contents"? Can you explain in the documentation what this is? All I am getting back is a []byte.

bufdev · 2024-09-03T16:11:02Z

sourceinfo/sourceinfo.go

+	"google.golang.org/protobuf/types/descriptorpb"
+)
+
+// GlobalFiles is a replacement for protoregistry.GlobalFiles that includes


Needs much more extensive documentation. As a reader, I don't know what "all registered source info" is, and re: comment above, I don't know how this replaces protoregistry.GlobalFiles (and I should not have to read the code to find out if it actually does or how it does)

bufdev · 2024-09-03T16:12:40Z

sourceinfo/sourceinfo.go

+var (
+	// GlobalFiles is a replacement for protoregistry.GlobalFiles that includes
+	// all registered source info.
+	GlobalFiles protodesc.Resolver = sourceinfo.GlobalFiles


This was not resolved - you just changed this from a global variable to a static function that wraps a global variable.

Alfus added 2 commits August 7, 2024 12:36

Add sourceinfo plugin

b79a31e

use compact

c00a0cd

Alfus requested a review from jhump August 7, 2024 17:16

Alfus added 2 commits August 7, 2024 13:33

lint

14d621d

nit

a87ee39

Alfus requested a review from rodaine August 7, 2024 17:35

Alfus added 2 commits August 7, 2024 13:39

fix windows test

f0d0ea7

try to fix windows again

7cd6b87

Alfus requested a review from mfridman August 7, 2024 17:49

Alfus added 7 commits August 7, 2024 13:53

add comments

0790ed3

nit

eb3a151

Add cmd

f26b23c

switch to .binpb

ca777e3

Add support for embedding SourceInfo

75dc2f1

lint

72fc2b8

nit

fc431ec

jhump reviewed Aug 7, 2024

View reviewed changes

cmd/version.go Outdated Show resolved Hide resolved

sourceinfo.go Outdated Show resolved Hide resolved

Alfus added 3 commits August 7, 2024 15:22

Switch to fs.FS

9c31ed9

re

d943794

re

2031d73

jhump approved these changes Aug 7, 2024

View reviewed changes

sourceinfo/sourceinfo.go Outdated Show resolved Hide resolved

re

911c540

Alfus requested a review from bufdev August 7, 2024 21:06

mfridman approved these changes Aug 7, 2024

View reviewed changes

mfridman reviewed Aug 7, 2024

View reviewed changes

sourceinfo/sourceinfo.go Outdated Show resolved Hide resolved

re

2c40417

Update readme

dc81810

bufdev requested changes Aug 8, 2024

View reviewed changes

Alfus added 9 commits August 8, 2024 13:10

re

e8511c2

nit

eb5b1d4

update readme

0792f84

re

e19188d

nit

1b01549

nit

e67d687

nit

e0f253b

Merge branch 'main' into alfus/sourceinfo

a116ab6

nit

2aa6f75

bufdev requested changes Sep 3, 2024

View reviewed changes

Alfus added 2 commits September 3, 2024 09:28

re

c3976c8

re

15f80d5

Alfus closed this Jan 2, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add sourceinfo plugin #22

Add sourceinfo plugin #22

Alfus commented Aug 7, 2024 •

edited

Loading

jhump left a comment

mfridman left a comment •

edited

Loading

Alfus commented Aug 7, 2024

mfridman commented Aug 7, 2024

bufdev Aug 8, 2024

Alfus Aug 8, 2024

bufdev Sep 3, 2024

bufdev Aug 8, 2024

bufdev Sep 3, 2024

Alfus Sep 3, 2024

bufdev Sep 3, 2024

Alfus Sep 3, 2024 •

edited

Loading

bufdev Sep 3, 2024

Alfus Sep 3, 2024

bufdev Sep 3, 2024

bufdev Sep 3, 2024

bufdev Sep 3, 2024


		The following helpers are provided for go via `import "github.com/bufbuild/protoschema-plugins/sourceinfo"`:

		- `sourceinfo.GlobalFiles()`: A drop-in replacement for protoregistry.GlobalFiles

Add sourceinfo plugin #22

Add sourceinfo plugin #22

Conversation

Alfus commented Aug 7, 2024 • edited Loading

jhump left a comment

Choose a reason for hiding this comment

mfridman left a comment • edited Loading

Choose a reason for hiding this comment

Alfus commented Aug 7, 2024

mfridman commented Aug 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Alfus Sep 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Alfus commented Aug 7, 2024 •

edited

Loading

mfridman left a comment •

edited

Loading

Alfus Sep 3, 2024 •

edited

Loading