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.

Sign up for GitHub

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

Jump to bottom

Restore protobuf patch to drop java 7 compatibility #478

Closed
wants to merge 2 commits into from
Closed

Restore protobuf patch to drop java 7 compatibility #478

wants to merge 2 commits into from

Conversation

davido
Copy link
Contributor

@davido davido commented Mar 15, 2020 •
edited
Loading

This partially reverts canges done in: 3bdfdbf.

As the consequence building downstream e.g. Gerrit Code Review re-introduced these annoying warnings:

INFO: From Building external/io_bazel_rules_closure/java/io/bazel/rules/closure/worker/libworker_protocol_proto-speed.jar (1 source jar) [for host]:
warning: -parameters is not supported for target value 1.7. Use 1.8 or later.
INFO: From Building external/io_bazel_rules_closure/java/io/bazel/rules/closure/webfiles/libbuild_info_proto-speed.jar (1 source jar) [for host]:
warning: -parameters is not supported for target value 1.7. Use 1.8 or later.
INFO: From Building external/io_bazel_rules_closure/java/io/bazel/rules/closure/libbuild_info_proto-speed.jar (1 source jar) [for host]:
warning: -parameters is not supported for target value 1.7. Use 1.8 or later.
Comment 1 by david.pursehouse@gmail.com on Tue, Jul 2, 2019, 3:03 AM GMT+2

Related Gerrit Issue: [1]. Related Bazel Issue: [2]. Rejected attempt to drop Java 7 compatibility in Protobuf: [3].

[1] https://bugs.chromium.org/p/gerrit/issues/detail?id=11102
[2] bazelbuild/bazel#8772
[3] protocolbuffers/protobuf#6711

@davido
Copy link
Contributor Author

davido commented Mar 15, 2020 •
edited
Loading

//CC @Yannic @alexeagle

@davido davido mentioned this pull request Mar 15, 2020
Closed
@davido davido force-pushed the protobuf-patch-to-drop-java-7-compatibility branch from 95e0451 to 157a3d7 Compare March 15, 2020 08:31
Yannic
Yannic approved these changes Mar 15, 2020
View reviewed changes
Copy link
Contributor

@Yannic Yannic left a comment

Choose a reason for hiding this comment

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

nit: Please add exports_files(["protobuf_drop_java_7_compatibility.patch"]) to //closure:BUILD

Sorry for deleting the patch in #474! LGTM

@davido
Copy link
Contributor Author

davido commented Mar 15, 2020

nit: Please add exports_files(["protobuf_drop_java_7_compatibility.patch"]) to //closure:BUILD

Done. Thanks for the review!

@davido davido force-pushed the protobuf-patch-to-drop-java-7-compatibility branch from 215a8a1 to bd04db8 Compare March 15, 2020 13:07
@gkdn gkdn mentioned this pull request Mar 19, 2020
Merged
qtprojectorg pushed a commit to qtqa/gerrit that referenced this pull request Mar 23, 2020
@davido
Update rules_closure to latest version
a0a6ff4 
This update upgrades Protobuf to v3.11.4 and replaces default value of
closure_js_binary#dependency_mode with PRUNE_LEGACY.

This upgrade switched to forked rules_closure version, until this PR
is merged: [1].

[1] bazelbuild/rules_closure#478

Change-Id: I6d4ac2ae24545f302fec9a39bbcd019974f574be
@davido
Copy link
Contributor Author

davido commented Apr 23, 2020

Ping? //CC @meteorcloudy

Can we we move forward here and create new rules_closure release and also publish it on Bazel Mirror?

Right now our CI is failing, because Gerrit build depends on my rules_closure fork, and GitHub is not reliable enough. see the whole story: [1].

[1] https://bugs.chromium.org/p/gerrit/issues/detail?id=12542

@davido
Copy link
Contributor Author

davido commented May 11, 2020

Thanks @acozzette protocolbuffers/protobuf#7306 is merged now and we don't have to patch Protobuf any more. Yeah!

@davido davido closed this May 11, 2020
@davido davido deleted the protobuf-patch-to-drop-java-7-compatibility branch July 17, 2022 07:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Yannic Yannic Yannic approved these changes

@gregmagolan gregmagolan Awaiting requested review from gregmagolan gregmagolan is a code owner

@soldair soldair Awaiting requested review from soldair soldair is a code owner

Assignees
No one assigned
Labels
cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@davido @Yannic @googlebot