-
-
Notifications
You must be signed in to change notification settings - Fork 258
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
Use Bazel's JDK to run coursier #1323
base: master
Are you sure you want to change the base?
Use Bazel's JDK to run coursier #1323
Conversation
If this looks like an acceptable approach I can update the documentation as well. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The general approach seems fine, but it might be fragile.
return repository_ctx.path(java_home + "/bin/java") | ||
elif repository_ctx.which("java") != None: | ||
return repository_ctx.which("java") | ||
embedded_java = "../bazel_tools/jdk/bin/java" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this work with Bazel 8? I thought the location of the java binary was somewhere under rules_java
now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It worked when I ran REPIN=1 bazel run @regression_testing_coursier//:pin
with Bazel 8.0.1. Adding RJE_VERBOSE=1 showed that the coursier tasks were running with
.../bazel_tools/jdk/bin/java -noverify -jar ...
I also deleted the bazel cache directory and ran it from a clean install and it also worked.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this JDK the minimized (jlinked) JDK Bazel bundles, or is it something else (e.g. a downloaded separate one)?
I'm asking because I think the minimized JDK may not contain all normal JDK modules, and so using it to run Coursier could be problematic down the road if the embedded JDK ends up not containing modules Coursier happens to need, which there isn't a good way to prevent, since I think Bazel's own JDK stripping assumes that JDK will only run Bazel and not other things.
When I check the module list for this JDK in a local Bazel install, it looks like it's been stripped down.
$ jimage list bazel_tools/jdk/lib/modules | grep "Module:"
Module: java.base
Module: java.compiler
Module: java.instrument
Module: java.logging
Module: java.management
Module: java.naming
Module: java.security.sasl
Module: java.sql
Module: java.transaction.xa
Module: java.xml
Module: jdk.crypto.ec
Module: jdk.management
Module: jdk.unsupported
as compared to a full JDK:
$ jimage list modules | grep "Module:"
Module: java.base
Module: java.compiler
Module: java.datatransfer
Module: java.desktop
Module: java.instrument
Module: java.logging
Module: java.management.rmi
Module: java.management
Module: java.naming
Module: java.net.http
Module: java.prefs
Module: java.rmi
Module: java.scripting
Module: java.se
Module: java.security.jgss
Module: java.security.sasl
Module: java.smartcardio
Module: java.sql.rowset
Module: java.sql
Module: java.transaction.xa
Module: java.xml.crypto
Module: java.xml
Module: jdk.accessibility
Module: jdk.attach
Module: jdk.charsets
Module: jdk.compiler
Module: jdk.crypto.cryptoki
Module: jdk.crypto.ec
Module: jdk.dynalink
Module: jdk.editpad
Module: jdk.graal.compiler.management
Module: jdk.graal.compiler
Module: jdk.hotspot.agent
Module: jdk.httpserver
Module: jdk.incubator.vector
Module: jdk.internal.ed
Module: jdk.internal.jvmstat
Module: jdk.internal.le
Module: jdk.internal.opt
Module: jdk.internal.vm.ci
Module: jdk.jartool
Module: jdk.javadoc
Module: jdk.jcmd
Module: jdk.jconsole
Module: jdk.jdeps
Module: jdk.jdi
Module: jdk.jdwp.agent
Module: jdk.jfr
Module: jdk.jlink
Module: jdk.jpackage
Module: jdk.jshell
Module: jdk.jsobject
Module: jdk.jstatd
Module: jdk.localedata
Module: jdk.management.agent
Module: jdk.management.jfr
Module: jdk.management
Module: jdk.naming.dns
Module: jdk.naming.rmi
Module: jdk.net
Module: jdk.nio.mapmode
Module: jdk.random
Module: jdk.sctp
Module: jdk.security.auth
Module: jdk.security.jgss
Module: jdk.unsupported.desktop
Module: jdk.unsupported
Module: jdk.xml.dom
Module: jdk.zipfs
private/rules/coursier.bzl
Outdated
embedded_java = "../bazel_tools/jdk/bin/java" | ||
if _is_file(repository_ctx, embedded_java): | ||
return repository_ctx.path(embedded_java) | ||
else: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We've already returned at this point if embedded_java
is a file, so the else
is unnecessary and introduces extra indentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, removed the else
private/rules/coursier.bzl
Outdated
java_home = repository_ctx.os.environ.get("JAVA_HOME") | ||
if java_home != None: | ||
return repository_ctx.path(java_home + "/bin/java") | ||
elif repository_ctx.which("java") != None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see we did the same thing here, but I don't think it makes things clearer. shrug
@@ -120,6 +121,9 @@ pin_dependencies = rule( | |||
doc = "Location of the generated lock file", | |||
mandatory = True, | |||
), | |||
"jvm_flags": attr.string( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A string_list
would better model what you're doing here, and would match how we set jvm args in the regular java rules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, but is there value in parsing the JDK_JAVA_OPTIONS
env var into a string_list only to immediately re-concatenate it so we can pass it to the --jvm_flags
option of the java wrapper script?
b4a815e
to
48176f4
Compare
Since this would change the default java running coursier do we want to provide an option for people to opt back to using JAVA_HOME if they need to? |
@cheister, that's probably a good idea. The neatest way would be a configurable build attribute, but an env variable would be simpler. |
Addresses #445 and #1046 by taking the workaround in #445 and applying it as the default before looking at JAVA_HOME.
I also added a way to give the JVM settings for both coursier and maven if a custom cacerts file was needed. This can be set by doing
in
.bazelrc
Based on JDK_JAVA_OPTIONS