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

Protobuf: Remove java 7 compatibility mode #432

Merged

Conversation

davido
Copy link
Contributor

@davido davido commented Oct 7, 2019

This is a workaround to suppress this annoying warning:

warning: -parameters is not supported for target value 1.7. Use 1.8 or later.

wenn building protobuf_java. The full story can be seen in this Bazel
issue: [1]. I tried hard to avoid the need for patching Protobuf on
fetch, by trying to remove Java 7 compatibility mode upstream, that was
rejected: [2]. Moreover, I tried hard and spent quite some time to fix
the problem in Bazel: [3], in non-invasive way, but this PR was as well
rejected. The next natural approach is to just patch the Protobuf source
during the fetch operation.

Needless to say, that the patching during the fetch has its own
disavdantages, that the patch would probably need to be updated on
every Protobuf upgrade. But what can you do? The developers are
complaining and filling the issues in every issue tracker, as here in
Gerrit Code Review project: [4].

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

This is a workaround to suppress this annoying warning:

  warning: -parameters is not supported for target value 1.7. Use 1.8 or later.

wenn building protobuf_java. The full story can be seen in this Bazel
issue: [1]. I tried hard to avoid the need for patching Protobuf on
fetch, by trying to remove Java 7 compatibility mode upstream, that was
rejected: [2]. Moreover, I tried hard and spent quite some time to fix
the problem in Bazel: [3], in non-invasive way, but this PR was as well
rejected. The next natural approach is to just patch the Protobuf source
during the fetch operation.

Needless to say, that the patching during the fetch has its own
disavdantages, that the patch would probably need to be updated on
every Protobuf upgrade. But what can you do? The developers are
complaining and filling the issues in every issue tracker, as here in
Gerrit Code Review project: [4].

[1] bazelbuild/bazel#8772
[3] bazelbuild/bazel#9494
[3] protocolbuffers/protobuf#6711
[4] https://bugs.chromium.org/p/gerrit/issues/detail?id=11102
@alexeagle alexeagle force-pushed the protobuf_remove_java_7_compatibility_mode branch from 6b98203 to 9ba15ca Compare October 11, 2019 20:28
@alexeagle
Copy link
Contributor

Thanks for the detailed explanation. I agree that you've exhausted the better avenues for fixing this so I'm fine carrying a patch.

Let's discuss separately whether I can migrate gerrit off of rules_closure, we can probably build the same thing just using the google-closure-compiler npm package.

@alexeagle alexeagle merged commit 18f8acf into bazelbuild:master Oct 11, 2019
@davido
Copy link
Contributor Author

davido commented Oct 12, 2019

@alexeagle Thanks for the approval!

Let's discuss separately whether I can migrate gerrit off of rules_closure, we can probably build the same thing just using the google-closure-compiler npm package.

+1. Gerrit Front End team located in Google@MUC is working to upgrade
Gerrit Code Review FE from Polymer 1 to Polymer 2 and it is almost ready,
and the next Gerrit release Gerrit 3.1 would be switched to Polymer 2.

Moreover we would like to re-deisgn current Gerrit frontend build tool chain machinery.
This is the gerrit issue: [1]. Your help would be greatly appreciated!

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

lucamilanesio pushed a commit to GerritCodeReview/gerrit that referenced this pull request Oct 13, 2019
The latest version removed Java 7 compatibility mode [1] for
protobuf_java rule.

Note that this above fix is only related to the building protobuf_java
rule itself, and is not related to the building of java_proto_library,
that is also affected, but for another reason: default java toolchain
in currently used java_tools distribution is also targeting Java
language level 7 for proto rules. Luckily my PR was accepted here and
Java 7 compatibility mode was removed. However, to take effect a new
java_tools version must be released and Bazel must be updated to point
to it. This will be done in the follow-up change.

[1] bazelbuild/rules_closure#432

Bug: Issue 11102
Change-Id: I0860772dbf78a397f1f8f78f17eb8dcec4ac07c0
@davido davido deleted the protobuf_remove_java_7_compatibility_mode branch October 14, 2019 06:16
sgammon pushed a commit to Bloombox/rules_closure that referenced this pull request Oct 17, 2019
This is a workaround to suppress this annoying warning:

  warning: -parameters is not supported for target value 1.7. Use 1.8 or later.

wenn building protobuf_java. The full story can be seen in this Bazel
issue: [1]. I tried hard to avoid the need for patching Protobuf on
fetch, by trying to remove Java 7 compatibility mode upstream, that was
rejected: [2]. Moreover, I tried hard and spent quite some time to fix
the problem in Bazel: [3], in non-invasive way, but this PR was as well
rejected. The next natural approach is to just patch the Protobuf source
during the fetch operation.

Needless to say, that the patching during the fetch has its own
disavdantages, that the patch would probably need to be updated on
every Protobuf upgrade. But what can you do? The developers are
complaining and filling the issues in every issue tracker, as here in
Gerrit Code Review project: [4].

[1] bazelbuild/bazel#8772
[3] bazelbuild/bazel#9494
[3] protocolbuffers/protobuf#6711
[4] https://bugs.chromium.org/p/gerrit/issues/detail?id=11102
@alexeagle
Copy link
Contributor

@davido sure, maybe we should chat about it in slack.bazel.build - I have cloned the gerrit repo and started looking at how to migrate but it's very complex. I'm starting with https://github.com/jleyba/js-dossier as a simpler example usage to find a simpler and better supported way.

@davido
Copy link
Contributor Author

davido commented Oct 18, 2019

@alexeagle I will attend upcoming Gerrit Hackathon in Sunnyvale (11th November - 15th November). May be you (or someone else from your team) could join us for a meeting to discuss the options?

I don't know who will attend from FE team from Google@MUC, though, but we could certainly do a vc.

@alexeagle
Copy link
Contributor

That sounds perfect. I remember meeting Shawn P long ago at a Jenkins hackathon around here when I think the idea for Gerrit was just starting. Let me know when is a good time during that hackathon to come by?

@davido
Copy link
Contributor Author

davido commented Oct 18, 2019

@lucamilanesio Can you help to plan 2-3 hours on site meeting with @alexeagle during the hackathon?

And send the invites to Ole Rehmsen and Ben Rohlfs, both from FE team from Google@MUC?

@lucamilanesio
Copy link

Sure, @alexeagle what date / time would be best for you? The hackathon will take place in the GerritForge office in Sunnyvale, 100 S Murphy Ave Suite 200, Sunnyvale, CA 94086, United States

ptmphuong pushed a commit to ptmphuong/rules_closure that referenced this pull request Dec 9, 2022
This is a workaround to suppress this annoying warning:

  warning: -parameters is not supported for target value 1.7. Use 1.8 or later.

wenn building protobuf_java. The full story can be seen in this Bazel
issue: [1]. I tried hard to avoid the need for patching Protobuf on
fetch, by trying to remove Java 7 compatibility mode upstream, that was
rejected: [2]. Moreover, I tried hard and spent quite some time to fix
the problem in Bazel: [3], in non-invasive way, but this PR was as well
rejected. The next natural approach is to just patch the Protobuf source
during the fetch operation.

Needless to say, that the patching during the fetch has its own
disavdantages, that the patch would probably need to be updated on
every Protobuf upgrade. But what can you do? The developers are
complaining and filling the issues in every issue tracker, as here in
Gerrit Code Review project: [4].

[1] bazelbuild/bazel#8772
[3] bazelbuild/bazel#9494
[3] protocolbuffers/protobuf#6711
[4] https://bugs.chromium.org/p/gerrit/issues/detail?id=11102
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants