[internal] Always use jars on the user classpath, and generalize transitive classpath building #13061

stuhood · 2021-10-01T04:20:32Z

As described in #13036, Jars are faster to capture and materialize (fewer inodes), and have historically been significantly more performant for the JVM to load. We should have the output of JVM compilation be jars, always.

Additionally, generalize the construction of the transitive classpath needed to run an application via a Addresses -> Classpath @rule.

Fixes #13036.

stuhood · 2021-10-01T04:20:45Z

Commits are useful to review independently.

chrisjrn

This seems generally fine. I've got some questions about using the new rules, which I'll ask out-of-band.

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

chrisjrn · 2021-10-01T15:15:58Z

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

+            artifact_requirements=(
+                ArtifactRequirements(
+                    [
+                        Coordinate(


I'm not sure why these are here? Can you either comment or move the junit dependencies to where they're needed?

Oops. Thought I had moved this, but instead it's just in both places. Good catch.

chrisjrn · 2021-10-01T15:16:42Z

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

+            Snapshot,
+            MergeDigests(
+                (
+                    prefixed_transitive_user_classfiles_digest,


Great, this is much simpler, thank you!

chrisjrn · 2021-10-01T15:22:57Z

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

+    # Jar.
+    # NB: We jar up the outputs in a separate process because the nailgun runner cannot support
+    # invoking via a `bash` wrapper (since the trailing portion of the command is executed by
+    # the nailgun server). We might be able to resolve this in the future via a Javac wrapper shim.


You may be able to use the jar utility here instead of directly calling zip -- it should be in there with the JDK. maybe nailgun will help there?

My main reason for not calling jar in the other PR was that I couldn't figure out what the classpath to the jar utility was, maybe you'll have more luck?

Yea... the same issue remains though: invoking two consecutive Java processes in the same sandbox won't work as it stands (because back in the day I thought that CLI parsing would be the way to go, rather than having an explicit type for this... not sure yet whether that was a mistake, but we'll see).

It's possible that it could be made to work in the future by changing up how nailgun is used to maybe have the nailgun client be an explicit process inside the sandbox, but there would be a fair amount to figure out there. We'll see. Right now we don't fork at all (the pants process just connects to the server directly with a rust client), which is about as fast as it gets.

chrisjrn · 2021-10-01T15:24:23Z

src/python/pants/engine/internals/graph.py

@@ -382,7 +382,9 @@ async def coarsened_targets(addresses: Addresses) -> CoarsenedTargets:
        if not any(component_address in addresses_set for component_address in component):
            continue
        component_set = set(component)
-        members = tuple(addresses_to_targets[a] for a in component)
+        members = tuple(


what was broken here? 👀

Even if the scc algorithm is stable, I don't really trust that the inputs will be stable (for example, if we enter the transitive graph at a different spot, etc). So mostly a precaution to uphold the deterministic member ordering guarantee for CoarsenedTarget.

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

[ci skip-build-wheels] [ci skip-rust]

[ci skip-rust] [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]

tdyas · 2021-10-01T17:30:03Z

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

+            argv=[
+                bash.path,
+                "-c",
+                " ".join(["cd", dest_dir, ";", zip_binary.path, "-r", f"../{output_file}", "."]),


Abundance of caution suggests using shlex.quote on the paths. But not a blocker.

Yea, good call. Most of these are literals, but not the zip_binary.path.

tdyas · 2021-10-01T17:30:45Z

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

@@ -91,6 +98,43 @@ def rule_runner() -> RuleRunner:
 )


+@dataclass(frozen=True)
+class RenderedClasspath:
+    """The contents of a classpath, organized as a key per entry with its contained classfiles."""


Is the key a filename of the jar?

Yea, correct. This is test-only, so I didn't apply quite as much rigor.

These were skipped when tests were re-enabled (#13046), because they had been broken by disabling sibling inference (#12978). Sibling inference is back on (temporarily: #13058), so they just need updates for JAR classpaths (#13061). [ci skip-rust] [ci skip-build-wheels]

* [internal] Run pyupgrade on src/python/pants/backend/python ([#13073](#13073)) * [internal] Re-enable some skipped JVM tests. ([#13074](#13074)) * [internal] Use `DownloadedExternalModules` when analyzing external Go packages ([#13076](#13076)) * [internal] Use `DownloadedExternalModules` during Go target generation ([#13070](#13070)) * [internal] Replace deprecated use of `[pytest] junit_xml_dir` with `[test] xml_dir. ([#13069](#13069)) * [internal] Add `DownloadedExternalModules` for Go ([#13068](#13068)) * [internal] Always use jars on the user classpath, and generalize transitive classpath building ([#13061](#13061)) * Add failing tests for Go external modules ([#13065](#13065)) * [internal] java: fix version in test ([#13064](#13064)) * [internal] Skip additional inference tests ([#13062](#13062)) * [internal] java: enable cycles for file-level targets generated by `java_sources` ([#13058](#13058)) * [internal] Add a `@logging` decorator for tests. ([#13060](#13060)) * [internal] Improve compatibility of nailgun with append only caches, and use them for Coursier ([#13046](#13046)) * [internal] Stop using `go.sum` when generating `_go_external_package` targets ([#13052](#13052)) * [internal] Rename `go_module` target to `go_mod` ([#13053](#13053)) * [internal] Refactor `go/util_rules/external_module.py` ([#13051](#13051)) * [internal] go: add analyzer and rules for test sources ([#13041](#13041)) * [Internal] Refactoring how we integrate with dockerfile ([#13027](#13027)) * [internal] Simplify `go/package_binary.py` ([#13045](#13045)) * [internal] Refactor `OwningGoMod` ([#13042](#13042)) * [internal] Refactor `go_mod.py` ([#13039](#13039)) * [internal] Record metadata on engine-aware params ([#13040](#13040)) * [internal] Test discovery of `go` binary ([#13038](#13038)) * [internal] Extract directory setup for terraform linters / formatters into a separate rule. ([#13037](#13037)) * [internal] java: register dependency inference rules ([#13035](#13035)) * [internal] Add `strutil.bullet_list()` to DRY formatting ([#13031](#13031)) * Minor cleanups for the autoflake linter / formatter. ([#13032](#13032)) * Ensure XML results recorded for both pytest and junit ([#13025](#13025)) * [internal] go: refactor compilation into separate rule ([#13019](#13019)) * [internal] go: refactor link step into separate rule ([#13022](#13022)) * [internal] go: enable plugin in repo and cleanup test project ([#13018](#13018)) * [internal] go: use colon to separate binary name and version ([#13020](#13020)) * [internal] tweak formatting of help text for sourcefile-validation subsystem. ([#13016](#13016)) * [internal] Use system-installed Go rather than installing via Pants ([#13007](#13007)) * Move the `process-execution-local-cleanup` hint to a more specific location. ([#13013](#13013)) * [internal] Split shell targets into atom vs generator ([#12957](#12957)) * Install Go in CI ([#13011](#13011)) * Refresh maintainers list. ([#13004](#13004)) * [internal] Refactor setup of GOROOT and `import_analysis.py` ([#13000](#13000)) * Infer dependencies on COPY of pex binaries for `docker_image`s. ([#12920](#12920)) * Prepare 2.7.0. ([#12995](#12995)) * [internal] jvm: skip JDK tests unless env var set ([#12994](#12994)) * [internal] jvm: limit caching of JDK setup processes ([#12992](#12992)) * [internal] Async-ify `NailgunPool::connect` and `nailgun::CommandRunner`. ([#12990](#12990)) * [internal] Replace `java_library` with `java_source` and `java_sources`, and add `java_test` ([#12976](#12976)) * Prepare 2.7.0rc5. ([#12987](#12987)) * [internal] terraform: refactor parser script into its own file ([#12985](#12985)) * [internal] jvm/java: ensure JDK downloaded in one process ([#12972](#12972)) * add JDK to GitHub Actions CI ([#12977](#12977)) * [internal] Re-enable the `clippy::used_underscore_binding` check. ([#12983](#12983)) * [internal] Use target generation for `_go_external_package` ([#12929](#12929)) * [internal] Bump CI token expiration threshold. ([#12974](#12974)) * [internal] Re-enable the Java backend. ([#12971](#12971)) * [internal] Implement `@union`s via `Query`s ([#12966](#12966)) * Remove `Enriched*Result` classes in favor of `EngineAwareReturnType.cacheable` ([#12970](#12970)) * [internal] Remove spurious `python_tests` directive ([#12968](#12968)) * [internal] Python coverage report generation uses precomputed addresses. ([#12965](#12965)) * Add PackageRootedDependencyMap for mapping inferred Java dependencies ([#12964](#12964))

[ci skip-rust] [ci skip-build-wheels]

#13061 changed the contract of `CompiledClassfiles` to hold either zero or one JAR file (without changing its name! oops: will do in a followup soon). Support for `scala` landed around the same time, and was not using JARs with `CompiledClassfiles`... which becomes relevant in #13329. [ci skip-rust] [ci skip-build-wheels]

#13506 and #13061 moved all usecases of `CompiledClassfiles` to producing and consuming JARs. To clarify the contract, and to prepare to produce this type in non-compilation positions in #13329, rename to `ClasspathEntry`. [ci skip-rust] [ci skip-build-wheels]

stuhood requested review from patricklaw, chrisjrn and tdyas October 1, 2021 04:20

stuhood mentioned this pull request Oct 1, 2021

[internal][WIP] Adds deploy_jar target type #13029

Merged

stuhood enabled auto-merge (squash) October 1, 2021 04:25

stuhood force-pushed the stuhood/transitive-classpath-construction branch from 54d5c1c to 9b00486 Compare October 1, 2021 05:07

chrisjrn approved these changes Oct 1, 2021

View reviewed changes

stuhood force-pushed the stuhood/transitive-classpath-construction branch from 9b00486 to f1d2770 Compare October 1, 2021 16:21

stuhood added 3 commits October 1, 2021 09:23

Extract a transitive classpath construction @rule.

1724165

[ci skip-build-wheels] [ci skip-rust]

Directly create a jar file during javac compilation.

8fd5197

[ci skip-rust] [ci skip-build-wheels]

Review feedback.

73c3fd5

# 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]

stuhood force-pushed the stuhood/transitive-classpath-construction branch from f1d2770 to 73c3fd5 Compare October 1, 2021 16:24

tdyas approved these changes Oct 1, 2021

View reviewed changes

stuhood merged commit f96beb8 into pantsbuild:main Oct 1, 2021

stuhood deleted the stuhood/transitive-classpath-construction branch October 1, 2021 18:32

stuhood mentioned this pull request Oct 1, 2021

[internal] Re-enable some skipped JVM tests #13074

Merged

wisechengyi mentioned this pull request Oct 2, 2021

2.8.0.dev3 prep #13081

Merged

stuhood added a commit to stuhood/pants that referenced this pull request Nov 6, 2021

Fixup scalac for pantsbuild#13061.

5a519be

[ci skip-rust] [ci skip-build-wheels]

stuhood mentioned this pull request Nov 6, 2021

[internal] Always produce/expect jars with scalac #13506

Merged

stuhood mentioned this pull request Nov 6, 2021

[internal] Rename CompiledClassfiles to ClasspathEntry. #13508

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[internal] Always use jars on the user classpath, and generalize transitive classpath building #13061

[internal] Always use jars on the user classpath, and generalize transitive classpath building #13061

stuhood commented Oct 1, 2021

stuhood commented Oct 1, 2021

chrisjrn left a comment

chrisjrn Oct 1, 2021

stuhood Oct 1, 2021

chrisjrn Oct 1, 2021

chrisjrn Oct 1, 2021

stuhood Oct 1, 2021 •

edited

Loading

chrisjrn Oct 1, 2021

stuhood Oct 1, 2021

tdyas Oct 1, 2021

stuhood Oct 1, 2021

tdyas Oct 1, 2021

stuhood Oct 1, 2021

[internal] Always use jars on the user classpath, and generalize transitive classpath building #13061

[internal] Always use jars on the user classpath, and generalize transitive classpath building #13061

Conversation

stuhood commented Oct 1, 2021

stuhood commented Oct 1, 2021

chrisjrn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuhood Oct 1, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuhood Oct 1, 2021 •

edited

Loading