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 google_protos and update protobuf to 0.14 #43

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

eliasdarruda
Copy link

@eliasdarruda eliasdarruda commented Jan 17, 2025

Closes #42

@mjheilmann mjheilmann self-requested a review January 18, 2025 17:05
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.

It appears that the CI configuration isn't at 100% and isn't letting me trigger it here, but I ran several checks locally:

  • mix test on the library application fails
  • mix deps.get in examples/helloworld fails, there is a dependency collision from the main library
  • when examples/helloworld is updated so mix dips.get succeeds, its mix test fails

A friendly reminder here to also check your PR locally and run tests when drafting PRs, you could have discovered some of these issues directly.

This PR is doing 2 separate actions and each have their own complexities. I request that you split this PR into two PRs so we can carefully ensure each change is in a good and mergable state.

Removing google_protos

This library relies on google_protos for its unit tests. Just removing the library without other changes causes multiple test cases to fail. The protobuf library performs a hack to have the definitions available for its own tests without depending on it explicitly. This dependency was added in #26

Updating protobuf from 0.12 to 0.14

There are breaking changes in this upgrade. These are visible through the examples/helloworld unit tests. php_generic_services is in use but appears to be removed, but the example also fails to reflect for additional reasons that I haven't dug into.

As an aside, having unit tests in the examples folder that cover different behaviors from the main library seems problematic, but I don't know that it's not simply failing for related reasons. I am interested if there are other issues lurking in this upgrade, or if there are simply flaws in the example and its tests.

@eliasdarruda
Copy link
Author

eliasdarruda commented Jan 18, 2025

My bad on not testing, the reason it is failing is because the compiled google protos from the protobuf package does not include the descriptor.

I've opened elixir-protobuf/protobuf#401 to track that. The concern here is that any package that depends on this lib cannot upgrade to the newest elixir-protobuf version because of this breaking change.

Once that is addressed there I'll revisit the tests here

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.

Google_protos dependency in conflict with protobuf modules.
2 participants