[internal] Improve compatibility of nailgun with append only caches, …

…and use them for Coursier (#13046)

Coursier is used to fetch JVMs and artifacts for Java support. On `main`, it uses its default cache directory, but as shown in #12293, this has concurrency issues when multiple clients are fetching JVMs at the same time.

The underlying issue is likely to be fixed by coursier/coursier#2197, but in the meantime we can move to using `append_only_caches` for the `coursier` `--cache-dir` and `--jvm-dir`. Alone, this isn't sufficient to avoid concurrency issues (since fetches would simply collide in a new location): but it allows us to re-configure the cache location in `RuleRunner` tests using the `[pytest] execution_slot_var` to prevent collisions.

Along the way, support for mixing `append_only_caches` and `use_nailgun` needed fixing.

Re-enables running of JVM tests by default, and fixes #12293.

pants.toml

@@ -106,6 +106,7 @@ extra_requirements.add = [
 ]
 lockfile = "3rdparty/python/lockfiles/pytest.txt"
 timeout_default = 60
+execution_slot_var = "EXECUTION_SLOT"
 
 [test]
 extra_env_vars = [

src/python/pants/backend/java/compile/javac.py

@@ -258,6 +258,7 @@ async def compile_java_source(
             ],
             input_digest=merged_digest,
             use_nailgun=jdk_setup.digest,
+            append_only_caches=jdk_setup.append_only_caches,
             output_directories=(dest_dir,),
             description=f"Compile {request.component.members} with javac",
             level=LogLevel.DEBUG,

src/python/pants/backend/java/compile/javac_test.py

@@ -61,10 +61,6 @@ def rule_runner() -> RuleRunner:
             QueryRule(CoarsenedTargets, (Addresses,)),
         ],
         target_types=[JvmDependencyLockfile, JavaSourcesGeneratorTarget, JvmArtifact],
-        # TODO(#12293): use a fixed JDK version.
-        bootstrap_args=[
-            "--javac-jdk=system",
-        ],
     )
 
 
@@ -160,7 +156,6 @@ def test_compile_no_deps(rule_runner: RuleRunner) -> None:
 
 
 @maybe_skip_jdk_test
-@pytest.mark.skip(reason="#12293 Coursier JDK bootstrapping is currently flaky in CI")
 def test_compile_jdk_versions(rule_runner: RuleRunner) -> None:
     rule_runner.write_files(
         {
@@ -208,7 +203,7 @@ def test_compile_jdk_versions(rule_runner: RuleRunner) -> None:
         rule_runner.request(CompiledClassfiles, [request])
 
 
-@maybe_skip_jdk_test
+@pytest.mark.xfail(reason="https://github.com/pantsbuild/pants/issues/13056")
 def test_compile_multiple_source_files(rule_runner: RuleRunner) -> None:
     rule_runner.write_files(
         {
@@ -455,7 +450,7 @@ class C implements A {}
     )
 
 
-@maybe_skip_jdk_test
+@pytest.mark.xfail(reason="https://github.com/pantsbuild/pants/issues/13056")
 def test_compile_with_transitive_multiple_sources(rule_runner: RuleRunner) -> None:
     """Like test_compile_with_transitive_cycle, but the cycle occurs via subtarget source expansion
     rather than explicitly."""

src/python/pants/backend/java/dependency_inference/import_parser_test.py

@@ -53,7 +53,6 @@ def rule_runner() -> RuleRunner:
             QueryRule(Targets, [UnparsedAddressInputs]),
         ],
         target_types=[JavaSourcesGeneratorTarget],
-        bootstrap_args=["--javac-jdk=system"],  # TODO(#12293): use a fixed JDK version.
     )
 
 

src/python/pants/backend/java/dependency_inference/java_parser.py

@@ -121,6 +121,7 @@ async def analyze_java_source_dependencies(
             input_digest=merged_digest,
             output_files=(analysis_output_path,),
             use_nailgun=tool_digest,
+            append_only_caches=jdk_setup.append_only_caches,
             description="Run Spoon analysis against Java source",
             level=LogLevel.DEBUG,
         ),

src/python/pants/backend/java/dependency_inference/java_parser_launcher.py

@@ -100,6 +100,7 @@ async def build_processors(bash: BashBinary, jdk_setup: JdkSetup) -> JavaParserC
             ],
             input_digest=merged_digest,
             use_nailgun=jdk_setup.digest,
+            append_only_caches=jdk_setup.append_only_caches,
             output_directories=(dest_dir,),
             description=f"Compile {_LAUNCHER_BASENAME} import processors with javac",
             level=LogLevel.DEBUG,

src/python/pants/backend/java/dependency_inference/java_parser_test.py

@@ -53,10 +53,6 @@ def rule_runner() -> RuleRunner:
             QueryRule(SourceFiles, (SourceFilesRequest,)),
         ],
         target_types=[JvmDependencyLockfile, JavaSourceTarget],
-        # TODO(#12293): use a fixed JDK version.
-        bootstrap_args=[
-            "--javac-jdk=system",
-        ],
     )
 
 

src/python/pants/backend/java/dependency_inference/rules_test.py

@@ -61,7 +61,6 @@ def rule_runner() -> RuleRunner:
             QueryRule(Targets, [UnparsedAddressInputs]),
         ],
         target_types=[JavaSourcesGeneratorTarget, JunitTestsGeneratorTarget],
-        bootstrap_args=["--javac-jdk=system"],  # TODO(#12293): use a fixed JDK version.
     )
 
 

src/python/pants/backend/java/test/junit.py

@@ -133,6 +133,7 @@ async def run_junit_test(
             ],
             input_digest=merged_digest,
             output_directories=(reports_dir,),
+            append_only_caches=jdk_setup.append_only_caches,
             description=f"Run JUnit 5 ConsoleLauncher against {field_set.address}",
             level=LogLevel.DEBUG,
         ),

src/python/pants/backend/java/test/junit_test.py

@@ -62,7 +62,6 @@ def rule_runner() -> RuleRunner:
             JunitTestsGeneratorTarget,
         ],
         bootstrap_args=[
-            "--javac-jdk=system",  # TODO(#12293): use a fixed JDK version.
             # Makes JUnit output predictable and parseable across versions (#12933):
             "--junit-args=['--disable-ansi-colors','--details=flat','--details-theme=ascii']",
         ],
@@ -584,87 +583,6 @@ class SimpleTest {
     assert re.search(r"1 tests found", test_result.stdout) is not None
 
 
-@maybe_skip_jdk_test
-def test_vintage_and_jupiter_simple_success(rule_runner: RuleRunner) -> None:
-    combined_lockfile = CoursierResolvedLockfile(
-        entries=(*JUNIT4_RESOLVED_LOCKFILE.entries, *JUNIT5_RESOLVED_LOCKFILE.entries)
-    )
-    rule_runner.write_files(
-        {
-            "coursier_resolve.lockfile": combined_lockfile.to_json().decode("utf-8"),
-            "BUILD": dedent(
-                """\
-                jvm_artifact(
-                  name='junit_junit',
-                  group='junit',
-                  artifact='junit',
-                  version='4.13.2',
-                )
-                jvm_artifact(
-                  name='org.junit.jupiter_junit-jupiter-api',
-                  group='org.junit.jupiter',
-                  artifact='junit-jupiter-api',
-                  version='5.7.2',
-                )
-                coursier_lockfile(
-                    name = 'lockfile',
-                    requirements = [
-                        ':junit_junit',
-                        ':org.junit.jupiter_junit-jupiter-api',
-                    ],
-                    sources = [
-                        "coursier_resolve.lockfile",
-                    ],
-                )
-
-                junit_tests(
-                    name='example-test',
-                    dependencies= [':lockfile'],
-                )
-                """
-            ),
-            "JupiterTest.java": dedent(
-                """
-                package org.pantsbuild.example;
-
-                import static org.junit.jupiter.api.Assertions.assertEquals;
-                import org.junit.jupiter.api.Test;
-
-                class JupiterTest {
-                    @Test
-                    void testHello(){
-                      assertEquals("Hello!", "Hello!");
-                   }
-                }
-                """
-            ),
-            "VintageTest.java": dedent(
-                """
-                package org.pantsbuild.example;
-
-                import junit.framework.TestCase;
-
-                public class VintageTest extends TestCase {
-                   public void testGoodbye(){
-                      assertTrue("Hello!" == "Hello!");
-                   }
-                }
-                """
-            ),
-        }
-    )
-
-    test_result = run_junit_test(rule_runner, "example-test", "JupiterTest.java")
-
-    assert test_result.exit_code == 0
-    # TODO: Once support for parsing junit.xml is implemented, use that to determine status so we can remove the
-    #  hack to use ASCII test output in the `run_junit_test` rule.
-    assert re.search(r"Finished:\s+testHello", test_result.stdout) is not None
-    assert re.search(r"Finished:\s+testGoodbye", test_result.stdout) is not None
-    assert re.search(r"2 tests successful", test_result.stdout) is not None
-    assert re.search(r"2 tests found", test_result.stdout) is not None
-
-
 def run_junit_test(
     rule_runner: RuleRunner, target_name: str, relative_file_path: str
 ) -> TestResult:

src/python/pants/backend/java/util_rules.py

@@ -25,6 +25,7 @@
 class JdkSetup:
     digest: Digest
     nailgun_jar: str
+    coursier: Coursier
     jdk_preparation_script: ClassVar[str] = "__jdk.sh"
     java_home: ClassVar[str] = "__java_home"
 
@@ -37,6 +38,10 @@ def args(self, bash: BashBinary, classpath_entries: Iterable[str]) -> tuple[str,
             ":".join([self.nailgun_jar, *classpath_entries]),
         )
 
+    @property
+    def append_only_caches(self) -> dict[str, str]:
+        return self.coursier.append_only_caches
+
 
 @rule
 async def setup_jdk(coursier: Coursier, javac: JavacSubsystem, bash: BashBinary) -> JdkSetup:
@@ -58,7 +63,9 @@ async def setup_jdk(coursier: Coursier, javac: JavacSubsystem, bash: BashBinary)
         coursier_jdk_option = "--system-jvm"
     else:
         coursier_jdk_option = f"--jvm={javac.options.jdk}"
-    java_home_command = f"{coursier.coursier.exe} java-home {coursier_jdk_option}"
+    java_home_command = " ".join(
+        coursier.args(["java-home", coursier_jdk_option, "--jvm-dir", f"{coursier.cache_dir}/jdk"])
+    )
 
     java_version_result = await Get(
         FallibleProcessResult,
@@ -69,6 +76,7 @@ async def setup_jdk(coursier: Coursier, javac: JavacSubsystem, bash: BashBinary)
                 f"$({java_home_command})/bin/java -version",
             ),
             input_digest=coursier.digest,
+            append_only_caches=coursier.append_only_caches,
             description=f"Ensure download of JDK {coursier_jdk_option}.",
             cache_scope=ProcessCacheScope.PER_RESTART_SUCCESSFUL,
         ),
@@ -116,6 +124,7 @@ async def setup_jdk(coursier: Coursier, javac: JavacSubsystem, bash: BashBinary)
             ),
         ),
         nailgun_jar=nailgun.file_name,
+        coursier=coursier,
     )
 
 

src/python/pants/backend/java/util_rules_test.py

@@ -50,6 +50,7 @@ def run_javac_version(rule_runner: RuleRunner) -> str:
                     "-version",
                 ],
                 input_digest=jdk_setup.digest,
+                append_only_caches=jdk_setup.append_only_caches,
                 description="",
             )
         ],

src/python/pants/backend/python/util_rules/pex_test.py

@@ -321,6 +321,9 @@ def test_pex_environment(rule_runner: RuleRunner, pex_type: type[Pex | VenvPex])
 
 @pytest.mark.parametrize("pex_type", [Pex, VenvPex])
 def test_pex_working_directory(rule_runner: RuleRunner, pex_type: type[Pex | VenvPex]) -> None:
+    named_caches_dir = (
+        rule_runner.options_bootstrapper.bootstrap_options.for_global_scope().named_caches_dir
+    )
     sources = rule_runner.request(
         Digest,
         [
@@ -387,10 +390,8 @@ def test_pex_working_directory(rule_runner: RuleRunner, pex_type: type[Pex | Ven
                 ["--backend-packages=pants.backend.python"],
                 env_inherit={"PATH", "PYENV_ROOT", "HOME"},
             )
+
             # Clear the cache.
-            named_caches_dir = (
-                rule_runner.options_bootstrapper.bootstrap_options.for_global_scope().named_caches_dir
-            )
             venv_dir = os.path.join(named_caches_dir, "pex_root", pex_data.pex.venv_rel_dir)
             assert os.path.isdir(venv_dir)
             safe_rmtree(venv_dir)

src/python/pants/jvm/resolve/coursier_fetch.py

@@ -237,16 +237,14 @@ async def coursier_resolve_lockfile(
     process_result = await Get(
         ProcessResult,
         Process(
-            argv=[
-                bash.path,
-                coursier.wrapper_script,
-                coursier.coursier.exe,
-                coursier_report_file_name,
-                *(req.to_coord_str() for req in artifact_requirements),
-            ],
+            argv=coursier.args(
+                [coursier_report_file_name, *(req.to_coord_str() for req in artifact_requirements)],
+                wrapper=[bash.path, coursier.wrapper_script],
+            ),
             input_digest=coursier.digest,
             output_directories=("classpath",),
             output_files=(coursier_report_file_name,),
+            append_only_caches=coursier.append_only_caches,
             description=(
                 "Running `coursier fetch` against "
                 f"{pluralize(len(artifact_requirements), 'requirement')}: "
@@ -340,17 +338,14 @@ async def coursier_fetch_one_coord(
     process_result = await Get(
         ProcessResult,
         Process(
-            argv=[
-                bash.path,
-                coursier.wrapper_script,
-                coursier.coursier.exe,
-                coursier_report_file_name,
-                "--intransitive",
-                request.coord.to_coord_str(),
-            ],
+            argv=coursier.args(
+                [coursier_report_file_name, "--intransitive", request.coord.to_coord_str()],
+                wrapper=[bash.path, coursier.wrapper_script],
+            ),
             input_digest=coursier.digest,
             output_directories=("classpath",),
             output_files=(coursier_report_file_name,),
+            append_only_caches=coursier.append_only_caches,
             description="Run coursier resolve",
             level=LogLevel.DEBUG,
         ),

src/python/pants/jvm/resolve/coursier_setup.py

@@ -5,7 +5,7 @@
 
 import textwrap
 from dataclasses import dataclass
-from typing import ClassVar
+from typing import ClassVar, Iterable
 
 from pants.core.util_rules.external_tool import (
     DownloadedExternalTool,
@@ -81,6 +81,15 @@ class Coursier:
     digest: Digest
     wrapper_script: ClassVar[str] = "coursier_wrapper_script.sh"
     post_processing_script: ClassVar[str] = "coursier_post_processing_script.py"
+    cache_name: ClassVar[str] = "coursier"
+    cache_dir: ClassVar[str] = ".cache"
+
+    def args(self, args: Iterable[str], *, wrapper: Iterable[str] = ()) -> tuple[str, ...]:
+        return tuple((*wrapper, self.coursier.exe, *args, "--cache", f"{self.cache_dir}"))
+
+    @property
+    def append_only_caches(self) -> dict[str, str]:
+        return {self.cache_name: self.cache_dir}
 
 
 @rule

src/python/pants/jvm/testutil.py

@@ -1,12 +1,12 @@
 # Copyright 2021 Pants project contributors (see CONTRIBUTORS.md).
 # Licensed under the Apache License, Version 2.0 (see LICENSE).
 
+import ast
 import os
 
 import pytest
 
 
 def maybe_skip_jdk_test(func):
-    return pytest.mark.skipif("PANTS_RUN_JDK_TESTS" not in os.environ, reason="Skip JDK tests")(
-        func
-    )
+    run_jdk_tests = bool(ast.literal_eval(os.environ.get("PANTS_RUN_JDK_TESTS", "True")))
+    return pytest.mark.skipif(not run_jdk_tests, reason="Skip JDK tests")(func)

src/python/pants/testutil/rule_runner.py

@@ -104,6 +104,13 @@ def __init__(
 
         bootstrap_args = [*bootstrap_args]
 
+        # TODO: Until https://github.com/coursier/coursier/pull/2197 is resolved, we avoid concurrent
+        # use of the named caches via `[pytest] execution_slot_var`.
+        bootstrap_args.append(
+            "--named-caches-dir="
+            f"{os.environ['HOME']}/.cache/pants/named_caches/tests/{os.environ['EXECUTION_SLOT']}"
+        )
+
         root_dir: Path | None = None
         if preserve_tmpdirs:
             root_dir = Path(mkdtemp(prefix="RuleRunner."))