jvm: use argfiles for JVM and JVM-compiler process arguments - #23620
jvm: use argfiles for JVM and JVM-compiler process arguments#23620robertpi wants to merge 4 commits into
Conversation
There was a problem hiding this comment.
Would you be able to expand on the cases where you hit this? Given where these limits generally sit it would have to be quite extreme to hit it for classpaths. Essentially on the order of thousands of classpath entries on a single target.
This PR effectively disables nailgun for JDK 9+, and nailgun is what gives warm JVMs, so that is a significant and unconditional cost on things like compile. But this isn't a problem when running nailgun in the first place, because then the potentially long arg list isn't passed via command arguments at all. So this disables nailgun, making it a universal problem, then applies the fix, and applies the file-indirection to every jvm_process whether or not it's needed. For anyone without the extreme classpath case that is strictly a regression.
The launcher args, the ones incompatible with nailgun, are also the ones that are bounded for anything nailgun-able. Under nailgun the only command line is the server spawn, carrying just the tool classpath. Everything that varies per invocation goes over the socket. So they are only unbounded for things that will not run under nailgun anyway, like tests.
The compile-side argfiles are nailgun-compatible, though also unnecessary under nailgun, since those args aren't passed via the command line there either.
So both are only needed when nailgun isn't used. The compile-side ones pay for that with a small unconditional overhead while the jvm_process one pays for it by disabling nailgun for everyone.
There is also a nailgun implementation that supports all modern JDK versions which Pants can be configured to use, so this would entirely disable the option of using that.
I would not like to see the blanket disabling of nailgun, but I can imagine other directions
- Only do this when nailgun is not used, which is not immediately known here. You would essentially need to predict if nailgun will be used, which needs to combine several pieces of state. This can easily drift.
- Doing it only if
use_nailgun=Falsewould allow for the fix when you know nailgun won't be used, but it being True only means it might. - Possibly rewriting the args at a later stage when you know how it will execute (nailgun or not) and would also know the actual argv length
| use_argfile = jdk.jre_major_version >= 9 and not contains_chroot_placeholder | ||
| use_nailgun = [] | ||
| if request.use_nailgun: | ||
| if request.use_nailgun and not use_argfile: |
There was a problem hiding this comment.
This effectively disables nailgun, where the fix is not needed since the long args are not passed on command line already.
There was a problem hiding this comment.
Fixed — see the full reply above and the new commit; gated the argfile on not request.use_nailgun instead of just the JDK version, so nailgun stays enabled here unless the caller already isn't going to use it.
|
Good catch — you're right this disables nailgun for every JDK 9+ process, not just the ones that need the argfile, and that nailgun already keeps the classpath off the command line entirely, so the fix bought nothing there and only cost something. Pushed a fix: the gate is now On the concrete case: it's Verified |
|
@jgranstrom good to go? |
|
My only reservation is that we still use the arg file unconditionally on the compile path. While the precedent is for running tests which never use nailgun, for that only the jvm_process path that is conditional is necessary, since tests run without nailgun and that is known there. So I'm wondering if we could scope out the compile paths at first, and possibly do the compile paths separately if they still end up hitting this issue? |
The Java 9+ argfile optimization moved the classpath and program
arguments into an `@__jvm_args.txt` file to avoid OS-level "Argument
list too long" errors. This broke `pants run` and deploy-jar execution
against a Java 9+ JDK: those code paths embed a literal `{chroot}`
placeholder in classpath entries (substituted by the engine only within
a process's argv at spawn time, never within file contents), and the
InteractiveProcess run in the workspace doesn't have its cwd set to the
sandbox, so the bare `@__jvm_args.txt` reference was also unresolvable.
Skip the argfile and fall back to inline argv whenever a `{chroot}`
placeholder is present in the classpath or argv, which is exactly the
set of callers (run.py, run_deploy_jar.py) relying on that
substitution. All other callers, which don't use `{chroot}`, keep using
the argfile.
Also updates a stale OpenAPI generator test that asserted on argv
content that now lives in the argfile, and adds a release note.
Nailgun sends the classpath and program arguments to the already-running server over a socket rather than via argv, so it was never subject to the OS argument-length limit the argfile works around, and gets no benefit from routing through it. Gating the argfile on JDK version alone unconditionally disabled nailgun for every Java 9+ process, including the common case (e.g. compilation) where nailgun was requested and the argfile wasn't needed. Gate on `not request.use_nailgun` instead, so the argfile is only used when the caller already isn't going to use nailgun (as JUnit/ScalaTest test execution and `pants run`/deploy-jar already do), rather than disabling nailgun universally to make room for it. Verified with `PY=python3.14 ./pants test` on: - src/python/pants/jvm/jdk_rules_test.py - src/python/pants/jvm/run_integration_test.py - src/python/pants/backend/openapi/util_rules/generator_process_test.py - src/python/pants/backend/java/compile/javac_test.py - src/python/pants/backend/scala/compile/scalac_test.py - src/python/pants/backend/kotlin/compile/kotlinc_test.py - src/python/pants/jvm/test/junit_test.py - src/python/pants/backend/scala/test/scalatest_test.py and `PY=python3.14 ./pants fmt lint check` on all changed files.
0023c68 to
a0098f7
Compare
The release note still described the javac/scalac/kotlinc compiler argfile change that was split out of this PR into a separate follow-up, since it wasn't tied to an observed failure the way the JvmProcess/nailgun fix was. Narrow the note to what this PR actually does now.
|
@jgranstrom okay, I agree the change to the compilers was speculative and quite possibly unnecessary, so I've removed it, so the change should just be scoped to the tests and another JVM process that's disabled Nailgun. |
3 participants
Problem
JVM processes that don't use nailgun — JUnit/ScalaTest test runs (
use_nailgun=False),pants run, and deploy-jar execution — invoke thejavabinary with their full classpath and argument list passed directly on the command line viaargv. For components with many dependencies or a long classpath, this can exceed OS-level command-line length limits (e.g.MAX_ARG_STRLEN/ARG_MAXon Linux/macOS), causing the process to fail with an "Argument list too long" style error.Concrete case hit in practice:
pants teston a JUnit 5 target with a ~250-jar classpath (Spark + Hadoop + Cassandra + Scala + Netty native bundles) failed withOs { code: 7, kind: ArgumentListTooLong, ... }when colon-joined into a single-cpargv element.Fix
JvmProcessnow writes thejavainvocation's arguments (classpath plus JVM options and program args) to a@argfile(__jvm_args.txt) instead of passing them on the command line, per thejavacommand-line argument files support added in JDK 9.not request.use_nailgun: nailgun sends the classpath and program arguments to the already-running server over a socket rather than via argv, so nailgun-eligible processes (e.g. compilation) were never subject to this limit and get no benefit from the argfile — they keep using nailgun exactly as before this PR.{chroot}placeholder is present in the classpath or argv. That placeholder is only substituted by the engine within aProcess's literalargvat spawn time, never within file contents, so callers relying on it (run.py,run_deploy_jar.py, which also run outside the sandbox viaInteractiveProcess) can't have those arguments moved into a file.Scoped out
An earlier version of this PR also routed the
javac/scalac/kotlinccompiler invocations' own argument lists through an@argfile, unconditionally. That change wasn't tied to an observed failure (compilation stays nailgun-eligible by default, so it doesn't hit this limit the same way), so it's been split out for separate review/justification rather than riding along with this fix.Test plan
src/python/pants/jvm/jdk_rules_test.py(new/updated coverage for argfile generation, escaping, the Java 9+ nailgun-vs-argfile branch, and the{chroot}fallback)src/python/pants/jvm/run_integration_test.py,src/python/pants/backend/openapi/util_rules/generator_process_test.py,src/python/pants/backend/java/compile/javac_test.py,src/python/pants/backend/scala/compile/scalac_test.py,src/python/pants/backend/kotlin/compile/kotlinc_test.py,src/python/pants/jvm/test/junit_test.py,src/python/pants/backend/scala/test/scalatest_test.py./pants lint fmt check teston all changed files