Skip to content

Conversation

dsyer
Copy link
Contributor

@dsyer dsyer commented Sep 26, 2025

Fixes #4629.

dsyer added a commit to dsyer/grammars-v4 that referenced this pull request Sep 26, 2025
Apparently the trailing comma is optional in blockLit, see
https://github.com/protocolbuffers/protobuf/blob/main/src/google/protobuf/descriptor.proto#L1026
for example. There are other examples in that file that use trailing commas
as list delimiters.

I think this change is probably also required by antlr#4631 to be fully correct,
but it is somewhat independent of that issue.
@kaby76
Copy link
Contributor

kaby76 commented Sep 26, 2025

You need to add .proto files to examples/. When I checked to see what was actually there, there is only one file (demo.proto). This is really quite unacceptable: one file cannot suffice for testing the grammar.

Copy link
Contributor

@kaby76 kaby76 left a comment

Choose a reason for hiding this comment

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

The change is fine; the tests work. The Antlr Maven Tester won't test the new example file, but that's ok for now. #4635

@teverett Please start the Github workflows for this PR. Ty.

09/26-07:25:30 ~/issues/g4-4629/protobuf/protobuf2
$ bash ../../_scripts/test.sh
Adding protobuf/protobuf2
Number of grammars before sorting and making unique: 1
Number of grammars after sorting and making unique: 1
Grammars to do are: protobuf/protobuf2
grammars = protobuf/protobuf2
targets = Antlr4ng CSharp Cpp Dart Go Java JavaScript Python3 TypeScript
order = grammars
filter = all
Tests now protobuf/protobuf2,Antlr4ng protobuf/protobuf2,CSharp protobuf/protobuf2,Cpp protobuf/protobuf2,Dart protobuf/protobuf2,Go protobuf/protobuf2,Java protobuf/protobuf2,JavaScript protobuf/protobuf2,Python3 protobuf/protobuf2,TypeScript

protobuf/protobuf2,Antlr4ng:
 Build 00:00:17
 Test 00:00:04
 Succeeded.

protobuf/protobuf2,CSharp:
 Build 00:00:08
 Test 00:00:03
 Succeeded.

protobuf/protobuf2,Cpp:
 Build 00:00:28
 Test 00:00:01
 Succeeded.

protobuf/protobuf2,Dart:
 Build 00:00:14
 Test 00:00:02
 Succeeded.

protobuf/protobuf2,Go:
 Build 00:01:12
 Test 00:00:02
 Succeeded.

protobuf/protobuf2,Java:
 Build 00:00:05
 Test 00:00:03
 Succeeded.

protobuf/protobuf2,JavaScript:
 Build 00:00:05
 Test 00:00:02
 Succeeded.

protobuf/protobuf2,Python3:
 Build 00:00:07
 Test 00:00:03
 Succeeded.

protobuf/protobuf2,TypeScript:
 Build 00:00:11
 Test 00:00:03
 Succeeded.
09/26-07:29:12 ~/issues/g4-4629/protobuf/protobuf2
$

teverett pushed a commit that referenced this pull request Sep 29, 2025
* [protobuf] Fix blockLit to allow trailing comma

Apparently the trailing comma is optional in blockLit, see
https://github.com/protocolbuffers/protobuf/blob/main/src/google/protobuf/descriptor.proto#L1026
for example. There are other examples in that file that use trailing commas
as list delimiters.

I think this change is probably also required by #4631 to be fully correct,
but it is somewhat independent of that issue.

* Add example with blockLits
@dsyer
Copy link
Contributor Author

dsyer commented Sep 30, 2025

I don't understand the CI failures and I can't figure out how to run them myself. Are they even relevant to this PR?

@kaby76
Copy link
Contributor

kaby76 commented Sep 30, 2025

Of the several build failures that I see, the build can't download the Antlr tool jar. This happens on a heavily loaded Github Action VM and/or Maven Cenral server. That causes timeouts. 'antlr4', which the scripts call, is not robust. I raised the issue years ago. antlr/antlr4-tools#13

It's not related to the PR.

At this point, we'll need to fork the tool repo and fix the problem. We are running out of options because development on Antlr has stopped, including bug fixes.

@kaby76
Copy link
Contributor

kaby76 commented Sep 30, 2025

Something has changed outside of the build scripts. The 'antlr4' tool returns:

Forbidden
HTTP code: 403

The URL is https://repo1.maven.org/maven2/org/antlr/antlr4/4.13.2/antlr4-4.13.2-complete.jar, which works fine in a browser. Doesn't help that Parr wrote the code to stuff all three exceptions into one misleading message.

@kaby76
Copy link
Contributor

kaby76 commented Sep 30, 2025

This pretty much sums up the problem: https://central.sonatype.org/faq/403-error-central/. The build is taking up too much of Maven Central. Going to have to change the antlr4-tools code to download from somewhere else, like github.com/antlr/antlr4/ , and update the grammars-v4 scripts to use the updated antlr4-tools.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[protobuf2] Extensions not parsed fully in Protobuf2

3 participants