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

Require protobuf only in dev/test #376

Closed
wants to merge 1 commit into from

Conversation

v0idpwn
Copy link
Contributor

@v0idpwn v0idpwn commented Jul 7, 2024

This fixes a potential regression on #351.

On README it's stated:

    # We don't force protobuf as a dependency for more
    # flexibility on which protobuf library is used,
    # but you probably want to use it as well

At @coingaming we rely on this characteristic of :grpc to be able to use an internal fork of :protobuf. Due to this change, we get a dependency conflict and aren't able to use :grpc v0.8+.

@@ -40,7 +40,7 @@ defmodule GRPC.Mixfile do
{:jason, ">= 0.0.0", optional: true},
{:cowlib, "~> 2.12"},
{:castore, "~> 0.1 or ~> 1.0", optional: true},
{:protobuf, "~> 0.11"},
{:protobuf, "~> 0.11", only: [:dev, :test]},
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this has already been changed before.

Can you check the history to confirm, please?

Simplest solution for you is to override: true on this dependency

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think can't override: true because I'm a package too :(

Will double check the history

I'd understand if its not accepted (but then the readme would need to be updated)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sleipnir was the change intentional? 👀

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like it breaks without this dependency, because of some new files that were added that use :protobuf in comp time...

Copy link
Collaborator

Choose a reason for hiding this comment

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

@v0idpwn Yes for implementing http transcoding we had to add some things, I'm not sure if it's possible to do without this. I may look into this in the future and try to reopen this issue if I see a solution.

@v0idpwn
Copy link
Contributor Author

v0idpwn commented Jul 8, 2024

Closing as it doesn't seem possible anymore. Will follow up with a PR removing the statement from the readme.

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.

3 participants