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

Fix parameters of java_common.compile. #7598

Merged
merged 3 commits into from Oct 8, 2021
Merged

Fix parameters of java_common.compile. #7598

merged 3 commits into from Oct 8, 2021

Conversation

comius
Copy link
Contributor

@comius comius commented Nov 6, 2020

Parameter host_javabase is removed

This is preparation for flipping incompatible_java_common_parameters.
See bazelbuild/bazel#12373

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Nov 6, 2020

CLA Check
The committers are authorized under a signed CLA.

@ejona86 ejona86 added the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Nov 6, 2020
@grpc-kokoro grpc-kokoro removed the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Nov 6, 2020
@comius
Copy link
Contributor Author

comius commented Nov 6, 2020

Failing because of extra old Bazel version, 1.0.1?

@ejona86
Copy link
Member

ejona86 commented Nov 6, 2020

@comius, yeah, we test on the oldest version we support (and we generally run newer versions on our machines when developing). What is the oldest version that supports this change? You can change the version used to test at:

use_bazel.sh 1.0.1

@ejona86
Copy link
Member

ejona86 commented Dec 9, 2020

It looks like this isn't compatible even with Bazel 3.7.1. It looks like we'll have to wait for 4.0, and even then we probably want to wait a while after 4.0 is out because changing this will force the new version no users.

I think it is probably best to close this for now. Agreed?

@comius
Copy link
Contributor Author

comius commented Dec 10, 2020

It looks like this isn't compatible even with Bazel 3.7.1. It looks like we'll have to wait for 4.0, and even then we probably want to wait a while after 4.0 is out because changing this will force the new version no users.

I think it is probably best to close this for now. Agreed?

True, it is only compatible with Bazel 4.0, which is going to be released hopefully this December :). Bazel 4.0 is a LTS release so it is taking a bit longer. It might not be such a bad idea to merge this after it is released or alternatively wait for next LTS.

I would not like to force you to have two versions because of Bazel. But I also will never be able to flip the flag if you insist on supporting all versions from 1.0.1.

If you agree I'd leave this open and ping you in a year when next LTS release will need the change. This way I don't need to search for it.

@ejona86
Copy link
Member

ejona86 commented Dec 10, 2020

Bumping the oldest supported version is fine. We test on the old version just so we don't accidentally break old versions. Bumping to 4.0 the day of the release is probably not great, but when the following LTS is coming out or a few months before seems quite fair.

Leaving this open sounds fine.

@comius comius marked this pull request as draft January 26, 2021 12:04
Parameter host_javabase is removed 

This is preparation for flipping incompatible_java_common_parameters.
See bazelbuild/bazel#12373
@comius comius marked this pull request as ready for review October 8, 2021 19:26
@comius
Copy link
Contributor Author

comius commented Oct 8, 2021

Bazel 4.x.x has been released now for 1 year. And we plan to do a cut off for Bazel 5.0.0 next week.

I would like to remove support for host_javabase in Bazel 5 by default.

@ejona86
Copy link
Member

ejona86 commented Oct 8, 2021

Oh, our CI is going to fail. @comius, could you update

use_bazel.sh 1.0.1
to use Bazel 4.0.0 instead? (Yes, we could use a newer version of Bazel in 4.x, but it is useful to know if we are hitting any bugs that would require even newer builds of Bazel. And we're bumping this from 1.0 so being a bit conservative doesn't seem a bad idea.)

@ejona86
Copy link
Member

ejona86 commented Oct 8, 2021

Actually, I can do that for you. Let me real quick so the CI starts running and we can get this done quickly for you.

@ejona86 ejona86 added the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Oct 8, 2021
@grpc-kokoro grpc-kokoro removed the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Oct 8, 2021
@ejona86 ejona86 merged commit bb51bb6 into grpc:master Oct 8, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 7, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants