Skip too new versioned directories of multi-release JARs when instrumenting by mlopatkin · Pull Request #25648 · gradle/gradle

mlopatkin · 2023-07-06T16:59:51Z

ASM library that we use for bytecode instrumentation has an upper limit to the bytecode version it can understand.
When processing multi-release JARs, the restriction caused errors even when running on the supported JVM, because instrumentation was processing all classes inside the JAR regardless of whether it was multi-release or not: e.g. JAR with versioned directories for Java 21 and Java 11 was still problematic even when running on Java 11 JVM, where new version of classes compiled for Java 21 aren't actually used.

This PR rewrites the instrumentation code to skip all versioned directories which aren't supported. The resulting instrumented JARs only contain versioned directories up to latest ASM-supported Java version. This way the cached transformed JAR is independent of the version of the JVM running Gradle.

However, there's a hole left when the build runs on the not-yet-supported-by-asm (and, therefore, Gradle) JVM, as it can then load non-instrumented classes from the versioned directories of the original JARs. This is going to be addressed in some follow-up PR.

Part of #25434.

octylFractal · 2023-07-07T00:09:02Z

I think this should have a test that the MRJAR entries are ignored, though I'm not sure how difficult that would be to add.

mlopatkin · 2023-07-10T15:53:37Z

I think this should have a test that the MRJAR entries are ignored, though I'm not sure how difficult that would be to add.

Thanks, I've added unit test coverage. Setting up a proper integration test is harder, I don't think there is enough ROI.

mlopatkin · 2023-07-11T17:55:20Z

@bot-gradle test RfR

bot-gradle · 2023-07-11T17:55:27Z

I've triggered the following builds for you. Click here to see all build failures.

ReadyForRelease build

bamboo

It's looking good overall. I made a few suggestions which could be addressed in a follow-up PR as needed.

bamboo · 2023-07-11T19:59:29Z

bamboo · 2023-07-11T20:05:06Z

🎨 isSignatureFile(entryName)

bamboo · 2023-07-11T20:07:09Z

🎨 Extract complex expression into something like isMultiReleaseJarManifestEntry(entry)?

bamboo · 2023-07-11T20:13:36Z

🧪 I wonder if it makes sense to introduce a test to ensure the constant matches the bundled asm 🤔

ASM doesn't expose the latest supported Java version anywhere but in the release notes. The closest thing I see it to declare a constant with the ASM version in AsmConstants and have a test that checks it against this one. Failing test may be a stronger nagging towards updating the constant.

I'll file an issue upstream in the ASM project, maybe they expose the info themselves - this would make things easier. For the test, it can be added when addressing ##25649.

bamboo · 2023-07-11T20:26:12Z

🚀 I'd avoid inspecting every entry name by making isMultiReleaseJar a nullable boolean and changing the predicate to isMultiReleaseJar == null && isManifestName(entryName)

I'd avoid inspecting every entry name by making isMultiReleaseJar a nullable boolean

I think we can even replace the signature file check with manifest check at this point, so we only need to look at the manifest and bail out of the entry for-each immediately. The manifest is typically the first JAR entry, which is nice. But this feels like out of scope for this PR. So far I applied your suggestion.

bamboo · 2023-07-11T20:45:14Z

🚀 Would it be possible to avoid the check once the manifest has been processed?

Would it be possible to avoid the check once the manifest has been processed?

We don't care about manifest here when transforming for the agent. We skip unsupported classes always: if the manifest has the attribute because they are unsupported, and if not - because they cannot be loaded as classes, only as resources. Maybe the comment above is a bit confusing and fails to convey this?

bamboo · 2023-07-11T20:49:03Z

❌ Spurious empty line

bamboo · 2023-07-11T20:52:28Z

🧪 I'd also include the assertContainsFile("Foo.class") assertion here for clarity.

bamboo · 2023-07-11T20:57:20Z

🎨 I had to pause to understand the checks multi-release jar manifest so I suggest renaming to something like legacy instrumentation preserves versioned resources in single-release jars (or non multi-release jars or jars without a multi-release attribute).

bamboo · 2023-07-11T21:01:39Z

❓ Any particular reason not to load the manifest on demand?

I intended to piggy-back on isManifestPresentAndFirstEntry initially, but discarded. Replaced with a lazy load (and made it possible to manifest even after creating the fixture with checkManifest = false).

This commit encapsulates all transformation logic into a separate class instead of pulling out pieces of it into the policy instance. Policy now serves as a factory for transformations. The refactoring paves way to extracting more info from JAR when preparing the instrumentation strategy.

ASM can only process class files up to a certain version. This can be problematic for multi-release JARs containing newer classes in the appropriate versioned directory. Such JARs can be used on a Gradle-supported JVM, but prior to this change, instrumentation failed even if the unsupported classes were never loaded. This commit makes instrumentation skip unsupported versioned directories. For legacy instrumentation, these directories are filtered out completely. For agent-based instrumentation, classes of the unsupported versions are not instrumented and aren't placed into the transformed archive. Unfortunately, ASM doesn't provide any kind of constant to specify the latest supported class file version, so we have to keep track of this manually.

In some cases JavaVersion class may come from a TAPI client which can be outdated and lack necessary constants.

mlopatkin · 2023-07-12T16:11:19Z

@bot-gradle test and merge

bot-gradle · 2023-07-12T16:11:29Z

I've triggered a build for you. Click here to see all failures if there's any.

This is a backport of #25648, but only the legacy instrumentation part. The agent-based instrumentation is only available in 8+.

mlopatkin added the @configuration-cache label Jul 6, 2023

mlopatkin added this to the 8.3 RC1 milestone Jul 6, 2023

mlopatkin requested review from a team and octylFractal July 6, 2023 16:59

mlopatkin requested a review from a team as a code owner July 6, 2023 16:59

mlopatkin self-assigned this Jul 6, 2023

mlopatkin requested a review from a team as a code owner July 10, 2023 14:39

mlopatkin force-pushed the ml/25434/skip-too-new-dirs-in-mrjars branch from e807519 to 90dca7f Compare July 10, 2023 15:52

octylFractal approved these changes Jul 10, 2023

View reviewed changes

mlopatkin changed the base branch from master to release July 11, 2023 17:55

bamboo approved these changes Jul 11, 2023

View reviewed changes

mlopatkin modified the milestones: 8.3 RC1, 8.4 RC1 Jul 12, 2023

mlopatkin changed the base branch from release to master July 12, 2023 15:28

mlopatkin added 5 commits July 12, 2023 17:32

Clarify some corner case in TransformReplacer

c21fba8

Do not use JavaVersion in AsmConstants

3b7d2b3

In some cases JavaVersion class may come from a TAPI client which can be outdated and lack necessary constants.

Add test coverage for multi-release jar instrumentation

80994cb

mlopatkin force-pushed the ml/25434/skip-too-new-dirs-in-mrjars branch from 90dca7f to 80994cb Compare July 12, 2023 15:32

gradle deleted a comment from mlopatkin Jul 12, 2023

bot-gradle merged commit 818a9ae into master Jul 12, 2023

bot-gradle deleted the ml/25434/skip-too-new-dirs-in-mrjars branch July 12, 2023 17:03

mlopatkin linked an issue Jul 13, 2023 that may be closed by this pull request

Instrumentation of multi-release JARs should not fail on newer versions #25434

Closed

mlopatkin added a commit that referenced this pull request Jan 9, 2024

Backport multi-release JAR fixes to 7.6.x line

0a06301

This is a backport of #25648, but only the legacy instrumentation part. The agent-based instrumentation is only available in 8+.

Conversation

mlopatkin commented Jul 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

octylFractal commented Jul 7, 2023

Uh oh!

mlopatkin commented Jul 10, 2023

Uh oh!

mlopatkin commented Jul 11, 2023

Uh oh!

bot-gradle commented Jul 11, 2023

Uh oh!

bamboo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mlopatkin commented Jul 12, 2023

Uh oh!

bot-gradle commented Jul 12, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mlopatkin commented Jul 6, 2023 •

edited

Loading