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

Bump Protobuf version #16890

Merged
merged 2 commits into from
Apr 6, 2023
Merged

Bump Protobuf version #16890

merged 2 commits into from
Apr 6, 2023

Conversation

ksobolew
Copy link
Contributor

@ksobolew ksobolew commented Apr 5, 2023

Description

Impersonating a dependabot here.

Additional context and related issues

Also removes unnecessary dependencyManagement in trino-pinot. Unless it's very important to keep it at a lower version?

Release notes

(x) This is not user-visible or docs only and no release notes are required.
( ) Release notes are required, please propose a release note for me.
( ) Release notes are required, with the following suggested text:

# Section
* Fix some things. ({issue}`issuenumber`)

The `protobuf-java` one was overriding a corresponding declaration in
the parent POM, and was effectively downgrading it. The other two were
not used at all.
@ksobolew
Copy link
Contributor Author

ksobolew commented Apr 5, 2023

Also removes unnecessary dependencyManagement in trino-pinot. Unless it's very important to keep it at a lower version?

I guess the protobuf-java was overridden there only because at the time it was added there was no "central" declaration in Trino parent POM. And then it just got forgotten. Is that correct @hashhar @elonazoulay?

@ksobolew ksobolew force-pushed the kudi/bump-protobuf branch from 0af376c to 06fe5b8 Compare April 5, 2023 13:17
@kokosing kokosing requested a review from elonazoulay April 5, 2023 13:27
@@ -34,26 +34,6 @@
</repository>
</repositories>

<dependencyManagement>
Copy link
Member

Choose a reason for hiding this comment

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

this section was added in https://github.com/trinodb/trino/pull/7627/files to avoid conflicting versions.

As long as enforcer doesn't complain about required upper bounds then it's good to remove.
I'd like @elonazoulay to confirm this once though.

Copy link
Contributor Author

@ksobolew ksobolew Apr 5, 2023

Choose a reason for hiding this comment

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

I checked, it doesn't complain (on older releases it complains about zookeeper only; seems most of this section is long obsolete)

@ksobolew
Copy link
Contributor Author

ksobolew commented Apr 6, 2023

@kokosing please merge

@hashhar hashhar merged commit 3723f52 into trinodb:master Apr 6, 2023
@hashhar hashhar added the no-release-notes This pull request does not require release notes entry label Apr 6, 2023
@ksobolew ksobolew deleted the kudi/bump-protobuf branch April 6, 2023 12:17
@github-actions github-actions bot added this to the 413 milestone Apr 6, 2023
@leetcode-1533
Copy link
Contributor

failed with "Unsupported platform: protoc-3.22.2-osx-aarch_64.exe" not sure if related

@ksobolew
Copy link
Contributor Author

failed with "Unsupported platform: protoc-3.22.2-osx-aarch_64.exe" not sure if related

Was it locally? M1 Mac? (I don't have one so I can't verify, but if protobuf doesn't have support for M1, then I don't know if I can do anything here.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed no-release-notes This pull request does not require release notes entry
Development

Successfully merging this pull request may close these issues.

4 participants