Skip to content

Add protobuf to requirements.#413

Merged
nwbirnie merged 5 commits intomasterfrom
nwbirnie-patch-1
Mar 24, 2021
Merged

Add protobuf to requirements.#413
nwbirnie merged 5 commits intomasterfrom
nwbirnie-patch-1

Conversation

@nwbirnie
Copy link
Copy Markdown
Contributor

Idea is to avoid issues like #412 in future.

Idea is to avoid issues like #412 in future.
@nwbirnie nwbirnie requested a review from devchas March 23, 2021 17:39
Comment thread README.md Outdated

* Java 1.8+
* Maven 3.0+
* Protobuf 3.12.2+
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Are you also seeing gRPC dependencies as an issue, or is protobuf by far the most common problem?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm not aware of a specific lower bound for gRPC. guava is a more common source of issues. I'll add this to the line up as well.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm not trying to complicate things (honest!), but would it be better if this was a separate section, or a "Dependencies" subsection of "Requirements"?

The protobuf and guava dependency requirements are different from the Java runtime requirement. The former only come into play if the user already includes protobuf or guava as a transitive or direct dependency in their project. If not, google-ads-java will work just fine. In contrast, if the user tries to use Java 1.5 🐯 , it's likely they'll run into issues.

Perhaps something like:

## Compatible dependencies

*  protobuf 3.12.2+
*  guava x.y.z+

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sounds good. Updated.

Comment thread README.md Outdated
Comment thread README.md Outdated

* Java 1.8+
* Maven 3.0+
* Protobuf 3.12.2+
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

How frequently do you anticipate this version will need to change? Wondering if it makes sense to automate keeping this in sync with poms, like you do with the library version on line 22.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good point, I'll add this to the release tasks for the next release.

Copy link
Copy Markdown
Member

@jradcliff jradcliff left a comment

Choose a reason for hiding this comment

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

One tiny nit, else LGTM

Comment thread README.md
@nwbirnie nwbirnie merged commit 50a6575 into master Mar 24, 2021
@nwbirnie nwbirnie deleted the nwbirnie-patch-1 branch March 24, 2021 13:52
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