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

Include proto unzip directory as proto import directory argument for protoc #43706

Merged
merged 2 commits into from
Oct 16, 2024
Merged

Include proto unzip directory as proto import directory argument for protoc #43706

merged 2 commits into from
Oct 16, 2024

Conversation

basselworkforce
Copy link
Contributor

@basselworkforce basselworkforce commented Oct 4, 2024

Relevant issue: #43539

This is to fix code gen issue where quarkus.generate-code.grpc.scan-for-proto-include option is used and .proto files use imports relative to the root directory.

Copy link

quarkus-bot bot commented Oct 4, 2024

Thanks for your pull request!

Your pull request does not follow our editorial rules. Could you have a look?

  • title should preferably start with an uppercase character (if it makes sense!)

This message is automatically generated by a bot.

@quarkus-bot quarkus-bot bot added the area/grpc gRPC label Oct 4, 2024
@basselworkforce basselworkforce changed the title io.quarkus:quarkus-grpc - Include proto unzip directory as proto import directory argument for protoc Include proto unzip directory as proto import directory argument for protoc Oct 4, 2024
@cescoffier cescoffier requested a review from alesj October 5, 2024 12:20
@gsmet
Copy link
Member

gsmet commented Oct 7, 2024

Rebased to avoid merge commits!

This comment has been minimized.

@basselworkforce
Copy link
Contributor Author

@alesj for the integration test that was failing I corrected the import path of the proto files so that they are relative to the root directory, otherwise we would have to change the sorting of the import directory arguments passed into protoc because protoc detects a conflicting .proto definition in the subfolder if it has already traversed the import directory root folder:

Fails:

...
-I=.../protoc-protos-from-dependencies/3a59f6ebb51a27a99eea361ddf0a831cb6314c7d 
-I=.../protoc-protos-from-dependencies/3a59f6ebb51a27a99eea361ddf0a831cb6314c7d/protobuf
...

with error:

protobuf/role.proto:12:11: "org.acme.protos.role.Role.role_id" is already defined in file "role.proto".
protobuf/role.proto:13:12: "org.acme.protos.role.Role.role_name" is already defined in file "role.proto".
protobuf/role.proto:11:9: "org.acme.protos.role.Role" is already defined in file "role.proto".

But this succeeds:

...
-I=.../protoc-protos-from-dependencies/3a59f6ebb51a27a99eea361ddf0a831cb6314c7d/protobuf 
-I=.../protoc-protos-from-dependencies/3a59f6ebb51a27a99eea361ddf0a831cb6314c7d
...

@basselworkforce
Copy link
Contributor Author

The import statements should use a path relative to the root as per the recommendation in the guidelines:

In general you should set the --proto_path flag to the root of your project and use fully qualified names for all imports.
https://protobuf.dev/programming-guides/proto3/#importing

@gsmet
Copy link
Member

gsmet commented Oct 9, 2024

What worries me a bit with this is that I thought this option was used to only handle some specific files/directories and from the error, it looks like it's now handling the whole root directory.
Is it expected?

Note that I'm not familiar at all with this thing so it might be a very dumb question.

@basselworkforce
Copy link
Contributor Author

basselworkforce commented Oct 9, 2024

no the change is only to pass in the root directory as an IMPORT_PATH to protoc (-I or --proto_path) which is used to resolve import statements: https://protobuf.dev/programming-guides/proto3/#generating
It does not impact which .proto files are processed (which are passed in sort of as varargs at the end).

Here is the complete command for reference in this example:

/home/brachid/quarkus/integration-tests/grpc-external-proto-test/target/com.google.protobuf-protoc-linux-x86_64-exe -I=/home/brachid/quarkus/integration-tests/grpc-external-proto-test/src/main/proto -I=/home/brachid/quarkus/integration-tests/grpc-external-proto-test/target/protoc-protos-from-dependencies/3a59f6ebb51a27a99eea361ddf0a831cb6314c7d -I=/home/brachid/quarkus/integration-tests/grpc-external-proto-test/target/protoc-protos-from-dependencies/3a59f6ebb51a27a99eea361ddf0a831cb6314c7d/protobuf -I=/home/brachid/quarkus/integration-tests/grpc-external-proto-test/target/protoc-protos-from-dependencies/3a59f6ebb51a27a99eea361ddf0a831cb6314c7d/dir -I=/home/brachid/quarkus/integration-tests/grpc-external-proto-test/target/protoc-dependencies/887869d68b3d4324376eb9ee6db4956483e8eaf8 --plugin=protoc-gen-grpc=/home/brachid/quarkus/integration-tests/grpc-external-proto-test/target/io.grpc-protoc-gen-grpc-java-linux-x86_64-exe --plugin=protoc-gen-q-grpc=/home/brachid/quarkus/integration-tests/grpc-external-proto-test/target/quarkus-grpc9349165086246327421.sh --q-grpc_out=/home/brachid/quarkus/integration-tests/grpc-external-proto-test/target/generated-sources/grpc --grpc_out=/home/brachid/quarkus/integration-tests/grpc-external-proto-test/target/generated-sources/grpc --java_out=/home/brachid/quarkus/integration-tests/grpc-external-proto-test/target/generated-sources/grpc /home/brachid/quarkus/integration-tests/grpc-external-proto-test/src/main/proto/extended.proto /home/brachid/quarkus/integration-tests/grpc-external-proto-test/target/protoc-protos-from-dependencies/3a59f6ebb51a27a99eea361ddf0a831cb6314c7d/protobuf/role.proto /home/brachid/quarkus/integration-tests/grpc-external-proto-test/target/protoc-protos-from-dependencies/3a59f6ebb51a27a99eea361ddf0a831cb6314c7d/protobuf/base.proto /home/brachid/quarkus/integration-tests/grpc-external-proto-test/target/protoc-protos-from-dependencies/3a59f6ebb51a27a99eea361ddf0a831cb6314c7d/dir/proto-with-java_generic_services.proto /home/brachid/quarkus/integration-tests/grpc-external-proto-test/target/protoc-protos-from-dependencies/3a59f6ebb51a27a99eea361ddf0a831cb6314c7d/dir/my-proto.proto

@gsmet
Copy link
Member

gsmet commented Oct 14, 2024

OK, looks good then. Let's see what CI has to say and merge if green.

@gsmet gsmet added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Oct 14, 2024
Copy link

quarkus-bot bot commented Oct 14, 2024

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 732c799.

Failing Jobs

Status Name Step Failures Logs Raw logs Build scan
JVM Tests - JDK 17 Build Failures Logs Raw logs 🔍
JVM Tests - JDK 21 Build Failures Logs Raw logs 🔍

You can consult the Develocity build scans.

Failures

⚙️ JVM Tests - JDK 17 #

- Failing: integration-tests/opentelemetry-jdbc-instrumentation 

📦 integration-tests/opentelemetry-jdbc-instrumentation

Failed to execute goal io.fabric8:docker-maven-plugin:0.45.1:start (docker-start) on project quarkus-integration-test-opentelemetry-jdbc-instrumentation: I/O Error


⚙️ JVM Tests - JDK 21 #

- Failing: integration-tests/opentelemetry-jdbc-instrumentation 

📦 integration-tests/opentelemetry-jdbc-instrumentation

Failed to execute goal io.fabric8:docker-maven-plugin:0.45.1:start (docker-start) on project quarkus-integration-test-opentelemetry-jdbc-instrumentation: I/O Error

@alesj
Copy link
Contributor

alesj commented Oct 15, 2024

@gsmet re-run? Since this doesn't look related ...

@gsmet gsmet merged commit 29b0b6b into quarkusio:main Oct 16, 2024
34 of 36 checks passed
@quarkus-bot quarkus-bot bot removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label Oct 16, 2024
@gsmet
Copy link
Member

gsmet commented Oct 16, 2024

We can merge, it's isolated to these two modules.

@gsmet
Copy link
Member

gsmet commented Oct 16, 2024

Thanks for the pull request!

@quarkus-bot quarkus-bot bot added this to the 3.16 - main milestone Oct 16, 2024
@basselworkforce
Copy link
Contributor Author

No problem! Thank you for the quick turnaround

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.

4 participants