Adds run support for deploy_jar targets
#14352
Conversation
# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
…re-downloading JDKs # Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
…ects # Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
chrisjrn
changed the title
run support for most deploy JARsrun support for deploy_jar targets
# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
| if not any( | ||
| filename.startswith("org.scala-lang_scala-library_") for filename in all_dependency_jars | ||
| ): | ||
| scala_library = await Get(ClasspathEntry, ScalaLibraryRequest(scala.version)) | ||
| direct_dependency_classpath_entries += (scala_library,) | ||
|
|
There was a problem hiding this comment.
I think that we should probably consider this to be a workaround for #14171, rather than a fix. If this ends up needing another iteration, adding a TODO here pointing to that ticket would be good.
| This is a hideous stop-gap, which will no longer be necessary once `InteractiveProcess` supports | ||
| append-only caches. |
There was a problem hiding this comment.
I've expanded #13852 to cover this: if this goes through another iteration, it would be good to link there.
| @rule(level=LogLevel.DEBUG) | ||
| async def create_deploy_jar_run_request( | ||
| field_set: DeployJarFieldSet, | ||
| runtime_jvm: __RuntimeJvm, |
There was a problem hiding this comment.
It's not clear how this ends up used... it's being captured, but I don't see anything re-writing the process execution to use it?
There was a problem hiding this comment.
| def prefixed(arg: str, prefixes: Iterable[str]) -> str: | ||
| if any(arg.startswith(prefix) for prefix in prefixes): | ||
| return f"{{chroot}}/{arg}" | ||
| else: | ||
| return arg | ||
|
|
||
| prefixes = (jdk_setup.bin_dir, jdk_setup.jdk_preparation_script, jdk_setup.java_home) | ||
| args = [prefixed(arg, prefixes) for arg in proc.argv] | ||
|
|
||
| env = { | ||
| **proc.env, | ||
| "PANTS_INTERNAL_ABSOLUTE_PREFIX": "{chroot}/", | ||
| } | ||
|
|
||
| # absolutify coursier cache envvars | ||
| for key in env: | ||
| if key.startswith("COURSIER"): | ||
| env[key] = prefixed(env[key], (jdk_setup.coursier.cache_dir,)) |
There was a problem hiding this comment.
Half of this is happening in the JDK support code, and the other half is happening here... it would be good for them to refer to one another with TODOs at least... possibly referencing #14386.
There was a problem hiding this comment.
The complication here is the number of places where we're doing preprocessing of process arguments. This will definitely become easier once we no longer have to rewrite things
|
Also, please adjust the PR title before landing. |
chrisjrn
changed the title
run support for deploy_jar targetsrun support for deploy_jar targets
# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
This adds
runsupport fordeploy_jartargets. It works! Closes #14283, at least untilwarfiles are properly supported.To make Scala binaries work, we now inject a dependency for the version of
scala-libraryspecified by--scala-versionat compile time, wheneverscala-libraryis not provided as a transitive dependency. This is a good-enough stop-gap in service of #14171.There's one ugly caveat which I'm reserving for future work:
For tool support, Couriser downloads a JVM into an append-only cache for most JVM processes. Since
InteractiveProcessdoes not support these, I've addedRuntimeJdk__, which downloads a JDK into aDigest. The digest gets written into the temporary directory alongside the runnable artifacts. It's not ideal, but for now, it removes the JDK download process from therunprocess, and the JDK is only downloaded once perpantsdexecution. It's probably ok for now. Being able to reuse the global append-only cache directories will be a better solution once we get there.