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

Unblock Bazel JDK 21 upgrade by patching RJE with upstream commit 5a78bc62 #21479

Closed
wants to merge 1 commit into from

Conversation

jin
Copy link
Member

@jin jin commented Feb 23, 2024

Context: bazel-contrib/rules_jvm_external#895 (comment)

This updates RJE to unblock building Bazel itself with Java 21.

@jin jin added the team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file. label Feb 23, 2024
@github-actions github-actions bot added the awaiting-review PR is awaiting review from an assigned reviewer label Feb 23, 2024
@jin jin force-pushed the bump-rje-2 branch 2 times, most recently from f85837e to b665125 Compare February 23, 2024 05:23
@jin
Copy link
Member Author

jin commented Feb 23, 2024

On the presubmit failures: the distdir logic seems to have an issue with looking for ~override module deps here (or the MODULE.bazel.lock is missing info for overriden deps?): https://cs.opensource.google/bazel/bazel/+/master:src/tools/bzlmod/utils.bzl;l=56-74;drc=64644991518fdd9eea61f0620e10a831a8848455

required_repos contains rules_jvm_external~override, but the moduleDepGraph entry for rules_jvm_external doesn't have a repoSpec: https://gist.github.com/jin/e5eb27cce566049fa62b2ba76a4c1563

@jin jin changed the title Update rules_jvm_external to use git_override (commit: 5a78bc62) Unblock Bazel JDK 21 upgrade by update rules_jvm_external to use archive_override on a prerelease commit (commit: 5a78bc62) Feb 23, 2024
@fmeum
Copy link
Collaborator

fmeum commented Feb 23, 2024

@jin So called "non-registry overrides" have special handling. Could you add the patch as a file and then add it to the existing single_version_override here

patches = ["//third_party:rules_jvm_external_6.0.patch"],

@fmeum
Copy link
Collaborator

fmeum commented Feb 23, 2024

The r_j_e patch doesn't look fully correct to me as it uses the Java compilation runtime, not the tool runtime configured via --tool_java_runtime_version. I will submit a follow-up PR upstream, the fix should be totally fine for use in Bazel though as-is.

@jin jin changed the title Unblock Bazel JDK 21 upgrade by update rules_jvm_external to use archive_override on a prerelease commit (commit: 5a78bc62) Unblock Bazel JDK 21 upgrade by patching RJE with upstream commit 5a78bc62 Feb 23, 2024
@jin
Copy link
Member Author

jin commented Feb 23, 2024

Thanks @fmeum, done.

@jin
Copy link
Member Author

jin commented Feb 23, 2024

Failures look legit on the arm64 Windows build:


(08:25:20) ERROR: C:/b/vzvul6zu/external/rules_jvm_external~6.0~maven~maven/BUILD:190:11: Stamping the manifest of @@rules_jvm_external~6.0~maven~maven//:com_google_api_client_google_api_client failed: (Exit -1): java.exe failed: error executing StampJarManifest command (from target @@rules_jvm_external~6.0~maven~maven//:com_google_api_client_google_api_client)
--
  | cd /d C:/b/vzvul6zu/execroot/_main
  | external\rules_java~7.4.0~toolchains~remotejdk11_win_arm64\bin\java.exe -jar bazel-out/x64_windows-opt-exec-ST-fad1763555eb/bin/external/rules_jvm_external~6.0/private/tools/java/com/github/bazelbuild/rules_jvm_external/jar/AddJarManifestEntry_deploy.jar --source bazel-out/x64_windows-fastbuild/bin/external/rules_jvm_external~6.0~maven~maven/com/google/api-client/google-api-client/1.35.2/google-api-client-1.35.2.jar --output bazel-out/x64_windows-fastbuild/bin/external/rules_jvm_external~6.0~maven~maven/com/google/api-client/google-api-client/1.35.2/processed_google-api-client-1.35.2.jar --manifest-entry Target-Label:@maven//:com_google_api_client_google_api_client
  | # Configuration: 544d1f532d3db342230291feefabc683d3a3eba40daccacb30eaf2cf7817b8b3
  | # Execution platform: //:default_host_platform
  | Action failed to execute: java.io.IOException: ERROR: src/main/native/windows/process.cc(202): CreateProcessW("C:\b\vzvul6zu\execroot\_main\external\rules_java~7.4.0~toolchains~remotejdk11_win_arm64\bin\java.exe" -jar bazel-out/x64_windows-opt-exec-ST-fad1763555eb/bin/external/rules_jvm_external~6.0/private/tools/java/com/github/bazelbuild/rules_jvm_external/jar/AddJarManifestEntry_deploy.jar --source bazel-out/x64_windows-fastbuild/bin/external/rules_jvm_external~6.0~maven~maven/com/google/api-client/google-api-client/1.35.2/google-api-client-1.35.2.jar --output bazel-out/x64_windows-fastbuild/bin(...)): This version of %1 is not compatible with the version of Windows you're running. Check your computer's system information and then contact the software publisher.
  | (error: 216)
  | (08:25:20) ERROR: C:/b/vzvul6zu/external/rules_jvm_external~6.0~maven~maven/BUILD:508:11: Stamping the manifest of @@rules_jvm_external~6.0~maven~maven//:com_google_code_findbugs_jsr305 failed: (Exit -1): java.exe failed: error executing StampJarManifest command (from target @@rules_jvm_external~6.0~maven~maven//:com_google_code_findbugs_jsr305)
  | cd /d C:/b/vzvul6zu/execroot/_main
  | external\rules_java~7.4.0~toolchains~remotejdk11_win_arm64\bin\java.exe -jar bazel-out/x64_windows-opt-exec-ST-fad1763555eb/bin/external/rules_jvm_external~6.0/private/tools/java/com/github/bazelbuild/rules_jvm_external/jar/AddJarManifestEntry_deploy.jar --source bazel-out/x64_windows-fastbuild/bin/external/rules_jvm_external~6.0~maven~maven/com/google/code/findbugs/jsr305/3.0.2/jsr305-3.0.2.jar --output bazel-out/x64_windows-fastbuild/bin/external/rules_jvm_external~6.0~maven~maven/com/google/code/findbugs/jsr305/3.0.2/processed_jsr305-3.0.2.jar --manifest-entry Target-Label:@maven//:com_google_code_findbugs_jsr305
  | # Configuration: 544d1f532d3db342230291feefabc683d3a3eba40daccacb30eaf2cf7817b8b3
  | # Execution platform: //:default_host_platform
  | Action failed to execute: java.io.IOException: ERROR: src/main/native/windows/process.cc(202): CreateProcessW("C:\b\vzvul6zu\execroot\_main\external\rules_java~7.4.0~toolchains~remotejdk11_win_arm64\bin\java.exe" -jar bazel-out/x64_windows-opt-exec-ST-fad1763555eb/bin/external/rules_jvm_external~6.0/private/tools/java/com/github/bazelbuild/rules_jvm_external/jar/AddJarManifestEntry_deploy.jar --source bazel-out/x64_windows-fastbuild/bin/external/rules_jvm_external~6.0~maven~maven/com/google/code/findbugs/jsr305/3.0.2/jsr305-3.0.2.jar --output bazel-out/x64_windows-fastbuild/bin/external/rules_jvm_e(...)): This version of %1 is not compatible with the version of Windows you're running. Check your computer's system information and then contact the software publisher.
  | (error: 216)

@fmeum
Copy link
Collaborator

fmeum commented Feb 23, 2024

The Windows arm64 build cross-compiles on amd64 for arm64. It looks like the jvm_import rule selects a Java runtime for the target platform, not the exec platform. Could you check whether my r_j_e patch fixes that?

@fmeum
Copy link
Collaborator

fmeum commented Feb 23, 2024

Ah, nvm, I still got this wrong in r_j_e. The runtime toolchain is selected for the target configuration. Instead, we could wrap the AddJarManifestEntry jar into a java_binary and depend on that in the exec cfg. No need for toolchains. Sorry for the confusion :⁠-⁠)

Edit: Not at a computer right now, but I can submit a fix for the fix later.

@fmeum
Copy link
Collaborator

fmeum commented Feb 23, 2024

@jin It actually looks like r_j_e had this correct before bazel-contrib/rules_jvm_external#1058. AddJarManifestEntry was a java_binary and depended on in the exec configuration. Maybe Bazel was just missing a --tool_java_runtime_version?

@jin
Copy link
Member Author

jin commented Feb 23, 2024

@fmeum dug into it a bit more - you're right, the original bazel patch just needed --tool_java_runtime_version=21, and the changes to RJE aren't necessary. Will revert.

@jin jin closed this Feb 23, 2024
@github-actions github-actions bot removed the awaiting-review PR is awaiting review from an assigned reviewer label Feb 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants