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

fix: support for bazel 7.x #16172

Closed
wants to merge 1 commit into from
Closed

fix: support for bazel 7.x #16172

wants to merge 1 commit into from

Conversation

sgammon
Copy link
Contributor

@sgammon sgammon commented Mar 14, 2024 •
edited
Loading

Summary

Fixes and closes #16169 by amending the build to support the latest version of Bazel (7.1.0). This is an alternative PR to #16171.

Tested against:

  • Bazel 7.1.0
  • Bazel 7.0.0
  • Bazel 6.3.2

On:

macOS 14.3.1 (aarch64)

Local Java:
openjdk version "11.0.19" 2023-04-18 LTS
OpenJDK Runtime Environment Zulu11.64+19-CA (build 11.0.19+7-LTS)
OpenJDK 64-Bit Server VM Zulu11.64+19-CA (build 11.0.19+7-LTS, mixed mode)

Changelog

  • fix: use to_list()[0] for depset
  • fix: upgrade rules_jvm_external → latest
  • fix: upgrade rules_kotlin → 1.9.0, to match rules_jvm_external
  • fix: io_bazel_rules_kotlin → rules_kotlin
  • fix: target for kotlin-stdlib
  • chore: re-generate maven dependency lockfile

@sgammon
fix: support for bazel 7.x
5af01ad 
Fixes and closes protocolbuffers#16169 by amending the build to
support the latest version of Bazel (`7.1.0`).

- fix: use `to_list()[0]` for depset
- fix: upgrade `rules_jvm_external` → latest
- fix: upgrade `rules_kotlin` → `1.9.0`, to match `rules_jvm_external`
- fix: `io_bazel_rules_kotlin` → `rules_kotlin`
- fix: target for `kotlin-stdlib`
- chore: re-generate maven dependency lockfile
@sgammon sgammon requested review from a team as code owners March 14, 2024 20:32
@sgammon sgammon requested review from haberman and shaod2 and removed request for a team March 14, 2024 20:32
@sgammon sgammon mentioned this pull request Mar 14, 2024
Closed
haberman
haberman reviewed Mar 14, 2024
View reviewed changes
java/kotlin/BUILD.bazel
@@ -50,10 +50,23 @@ kt_jvm_library(
deps = ["//java/core"],
)

java_import(
name = "kotlin-stdlib-jvm",
Copy link
Member

Choose a reason for hiding this comment

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

Why is this indirection necessary?

Copy link
Contributor Author

@sgammon sgammon Mar 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

It's not necessary, and I could remove it; the target changed within @com_github_jetbrains_kotlin, and I didn't want to expose the java_import etc functionality. I felt it was cleaner to have a single alias used in both //java/kotlin and //java/kotlin-lite.

But this is a matter of taste and I would defer to Google and/or other advice :)

Edit: The target should definitely not be public, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unless you mean the java_import? That is necessary because the target is now a JAR file, there is no kotlin-stdlib in that dependency anymore. Rules Kotlin was upgraded because Rules JVM was upgraded, which happened because the earlier version depends on toolchain internals which changed with Bazel 7.x.

Copy link
Contributor Author

@sgammon sgammon Mar 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Sorry, @haberman: maybe you didn't get these because I forgot to tag. My Mentions are where I'm usually looking.

@esrauchg esrauchg added the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Mar 22, 2024
@github-actions github-actions bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Mar 22, 2024
zhangskz
zhangskz reviewed Mar 25, 2024
View reviewed changes
java/osgi/osgi.bzl
@@ -174,7 +174,7 @@ def _osgi_jar_impl(ctx):
source_jars = source_jars.to_list()
if len(source_jars) > 1:
fail("osgi_jar rule doesn't know how to deal with more than one source jar.")
source_jar = target_java_output.source_jars[0]
source_jar = target_java_output.source_jars.to_list()[0]
Copy link
Member

Choose a reason for hiding this comment

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

You may need to rebase -- I believe this one was already fixed in 925de18

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you, will do.

@zhangskz
Copy link
Member

This seems to be failing CI tests with the following:

ERROR: /workspace/_build/out/external/local_jdk/BUILD.bazel:82:19: no such target '@bazel_tools//tools/jdk:bootstrap_runtime_toolchain_type': target 'bootstrap_runtime_toolchain_type' not declared in package 'tools/jdk' defined by /workspace/_build/out/external/bazel_tools/tools/jdk/BUILD (Tip: use `query "@bazel_tools//tools/jdk:*"` to see all the targets in that package) and referenced by '@local_jdk//:bootstrap_runtime_toolchain_definition'

@sgammon
Copy link
Contributor Author

sgammon commented Mar 25, 2024

@zhangskz Ah, that surfaces because rules_java needs to be upgraded. I will handle it in my rebase

@haberman
Copy link
Member

I think Bazel 7.x is working now. Can we close this PR?

@sgammon
Copy link
Contributor Author

sgammon commented May 12, 2024

I think so! Thanks @haberman

@sgammon sgammon closed this May 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@haberman haberman haberman left review comments

@zhangskz zhangskz zhangskz left review comments

@shaod2 shaod2 Awaiting requested review from shaod2 shaod2 is a code owner automatically assigned from protocolbuffers/protobuf-java

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Bazel build broken at main / 7.x
4 participants
@sgammon @zhangskz @haberman @esrauchg