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

remove submodule protobuf #26

Merged

Conversation

zhihuizhang17
Copy link
Contributor

@zhihuizhang17 zhihuizhang17 commented Feb 20, 2024

This PR removes the protobuf submodule and replaces it with the new dependency google_protos, as google_protos has been updated to v0.4.0 to support grpc_reflection.

@zhihuizhang17 zhihuizhang17 marked this pull request as ready for review February 20, 2024 07:51
Copy link
Collaborator

@mjheilmann mjheilmann left a comment

Choose a reason for hiding this comment

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

a few minor comments

mix.exs Outdated Show resolved Hide resolved
@@ -1,54 +0,0 @@
defmodule Google.Protobuf.Any do
Copy link
Collaborator

Choose a reason for hiding this comment

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

With these files removed, you can remove the line ~r/^Google\./, from the ignore_modules section of the root mix.exs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed.

@@ -42,7 +42,8 @@ defmodule GrpcReflection.MixProject do
{:credo, "~> 1.7", only: [:dev, :test], runtime: false},
{:dialyxir, "~> 1.4", only: [:dev, :test], runtime: false},
{:grpc, "~> 0.7"},
{:protobuf, "~> 0.11"}
{:protobuf, "~> 0.11"},
{:google_protos, "~> 0.4.0"}
Copy link
Collaborator

Choose a reason for hiding this comment

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

google_protos is only used in the unit tests and can be marked only: :test here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

google_protos is used for unit tests and example/helloworld. According to your suggestion, I will mark it only: :test and add the dependence to mix.exs in exmaple/helloworld.

Copy link
Contributor Author

@zhihuizhang17 zhihuizhang17 Feb 21, 2024

Choose a reason for hiding this comment

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

I added {:google_protos, "~> 0.4.0", only: :test} to grpc-reflection/mix.exs, then added {:google_protos, "~> 0.4.0"} to example/helloworld/mix.exs even with override: true, example/helloworld failed to run and threw following error.

11:57:00.494 [error] ** (CaseClauseError) no case clause matching: {:error, {:undef, [{Google.Protobuf.Timestamp, :descriptor, [], []},

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see that too, this is really unfortunate. Our use of the :google_protos package for testing is going to force all consuming libraries to also install and load the google protos modules whether they want to or not.

@mjheilmann mjheilmann merged commit 9754a08 into elixir-grpc:main Feb 21, 2024
1 check passed
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