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

Updated min tested Bazel version to 4.0.0 #533

Merged
merged 7 commits into from
Sep 30, 2021
Merged

Conversation

UebelAndre
Copy link
Contributor

@UebelAndre UebelAndre commented Sep 13, 2021

PR Checklist

Please check if your PR fulfills the following requirements:

  • Does not include precompiled binaries, eg. .par files. See CONTRIBUTING.md for info
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature (please, look at the "Scope of the project" section in the README.md file)
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • Other... Please describe:

What is the current behavior?

Issue Number: #526

What is the new behavior?

Updates the min tested Bazel version to 4.0.0

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

closes #526

Copy link
Contributor

@hrfuller hrfuller left a comment

Choose a reason for hiding this comment

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

I'm +1 on this, will wait for @alexeagle stamp as well. How risky is this upgrade, it may make future releases incompatible for people who can't go to newer bazel version right?

@alexeagle
Copy link
Contributor

We'll need to document this as a breaking change, that users must upgrade to 4.0. We'll start using new features like the json module which will fail. It has been the LTS version for a while, and I think we are not required to support older ones, so policy-wise I believe this is fine. (We ought to have some testing against rolling releases as well. I hope the rules authors SIG will make some guidance about what rulesets are expected to do)

One thing to figure out (maybe before merging this?) is how to give the right error message so developers know they must upgrade, instead of just reporting name 'json' is not defined
@UebelAndre could you find the right spot to hook up a versions.check https://docs.aspect.dev/bazelbuild/bazel-skylib/1.0.3/docs/versions.html#versions-check
?

@UebelAndre
Copy link
Contributor Author

I unfortunately don't have a whole lot of bandwidth so don't think I'll be able to get to that in the near future 😞 Is this something I can leave to the rules_python maintainers?

@alexeagle
Copy link
Contributor

I think I could find time for it. I'm looking around for where the check could go - there's no single call from users WORKSPACE files into a repository rule that we could put it. So I guess we just check at the beginning of several of them like pip_install and pip_import

@google-cla
Copy link

google-cla bot commented Sep 16, 2021

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@google-cla google-cla bot added cla: no and removed cla: yes labels Sep 16, 2021
@alexeagle
Copy link
Contributor

@googlebot I consent.

@google-cla google-cla bot added cla: yes and removed cla: no labels Sep 16, 2021
Since we only test against bazel 4.0 now, and will start using features that require it, this gives users
an obvious error message that they need to update.
@@ -3,6 +3,9 @@
load("@bazel_tools//tools/build_defs/repo:http.bzl", "http_archive")
load("@bazel_tools//tools/build_defs/repo:utils.bzl", "maybe")

# Avoid a load from @bazel_skylib repository as users don't necessarily have it installed
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

thanks for fixing it!

@google-cla
Copy link

google-cla bot commented Sep 18, 2021

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@google-cla google-cla bot added cla: no and removed cla: yes labels Sep 18, 2021
@hrfuller
Copy link
Contributor

@googlebot I consent.

@google-cla google-cla bot added cla: yes and removed cla: no labels Sep 18, 2021
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.

Increase min tested Bazel version to 4.0.0
3 participants