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

Allow extra option locations in source info when building an image #2803

Merged
merged 2 commits into from
Mar 6, 2024

Conversation

jhump
Copy link
Member

@jhump jhump commented Mar 6, 2024

This is a feature I added to protocompile last year because it was easy to do because of the way that protocompile parses the option values.

(It can't easily be done in protoc because it doesn't actually parse message literals: it scans through the source to the end of the value, looking for a matching { that closes the opening }, and just stores the tokens in a string value. Later, the value is interpreted by using the Protobuf text format to unmarshal that string of tokens into a message.)

Apparently, I forgot to enable it in buf last year. I did this specifically for things like linting custom options (like the protovalidate options), and we can see it pays off a bit in the updated test.

The motivation for me remembering to add this today, so many months later, is so that we can capture more of the comments when rendering custom options in generated docs.

@jhump jhump requested a review from bufdev March 6, 2024 21:14
@jhump
Copy link
Member Author

jhump commented Mar 6, 2024

cc @gilwong00

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.

Can you run buf build -o tmp.bin on a sufficiently large repo before/after to see how big the diff is in size?

@jhump
Copy link
Member Author

jhump commented Mar 6, 2024

Can you run buf build -o tmp.bin on a sufficiently large repo before/after to see how big the diff is in size?

@bufdev, here's buf.build/googleapis/googleapis.

-rw-r--r--  1 jhumphries  staff  344112 Mar  6 16:53 after.bin
-rw-r--r--  1 jhumphries  staff  330527 Mar  6 16:52 before.bin

That's about 4% difference. I'm happy to run this on any other repos, if you can think of other good examples I should try.

@bufdev
Copy link
Member

bufdev commented Mar 6, 2024

This is fine. Make sure this is merged into bufmod when you're done.

@jhump
Copy link
Member Author

jhump commented Mar 6, 2024

@mfridman suggested buf.build/beta/googleapis and buf.build/envoyproxy/envoy, too. Here are the updated numbers:

-rw-r--r--  1 jhumphries  staff   3271769 Mar  6 18:10 envoy-after.bin
-rw-r--r--  1 jhumphries  staff   3222085 Mar  6 18:12 envoy-before.bin
-rw-r--r--  1 jhumphries  staff  25166115 Mar  6 18:13 full-googleapis-after.bin
-rw-r--r--  1 jhumphries  staff  24874137 Mar  6 18:13 full-googleapis-before.bin
-rw-r--r--  1 jhumphries  staff    344112 Mar  6 16:53 slim-googleapis-after.bin
-rw-r--r--  1 jhumphries  staff    330527 Mar  6 16:52 slim-googleapis-before.bin

So 1.5% growth, 1.1%, and 4.1% respectively. Interestingly, the slimmed-down googleapis actually had the biggest delta. Average of 1.2% increase.

@jhump
Copy link
Member Author

jhump commented Mar 6, 2024

Make sure this is merged into bufmod when you're done.

Aye, aye!

@jhump jhump merged commit 49066c4 into main Mar 6, 2024
11 checks passed
@jhump jhump deleted the jh/extra-option-locations-in-source-info branch March 6, 2024 23:23
jhump added a commit that referenced this pull request Mar 6, 2024
…2803)

This opts into a feature in protocompile to produce more source code
locations (for span and comment info) than protoc produces. This
allows us to potentially report lint warnings with more precise position
information, and it should also allows us to carry more comment
information about custom option values into generated docs.
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