Skip to content

Build Reliability Upgrade: Cross-project runtimeClasspath resolution in geode-java.gradle#7925

Merged
raboof merged 2 commits into
apache:developfrom
JinwooHwang:runtimeClasspath
Oct 24, 2025
Merged

Build Reliability Upgrade: Cross-project runtimeClasspath resolution in geode-java.gradle#7925
raboof merged 2 commits into
apache:developfrom
JinwooHwang:runtimeClasspath

Conversation

@JinwooHwang

Copy link
Copy Markdown
Contributor

Summary

This pull request removes a cross-project configuration resolution in geode-java.gradle that was triggering Gradle warnings of the form:

Resolution of the configuration :geode-unsafe:runtimeClasspath was attempted from a context different 
than the project context. Have a look at the documentation to understand why this is a problem and how 
it can be resolved. This behaviour has been deprecated and is scheduled to be removed in Gradle 8.0.

Key Change

File: geode-java.gradle

Replaced this block (removed):

  • Retrieval of ResolutionResult from geodeProject.configurations.runtimeClasspath
  • Iteration over resolutionResult.allComponents to derive upstream dependency JAR names

With:

  • Metadata-only iteration over the already collected declared (first-level) runtime dependencies (depMap)
  • Construction of anticipated upstream JAR names using name-version.jar without forcing resolution

Motivation

Gradle 7+ warns—and Gradle 8 will fail—when a project resolves another project’s configurations during the configuration phase. This was producing noisy deprecation warnings during standard builds (e.g. :geode-core:jar) and risks breakage on future Gradle upgrades.

Removing this eager resolution:

  • Reduces build noise
  • Moves us toward Gradle 8 compatibility
  • Keeps logic deterministic without unnecessary dependency graph traversal

Testing & Validation Recommendations

Run:

./gradlew :geode-core:jar --warning-mode=all

Expected:

  • Prior runtimeClasspath cross-project resolution warnings no longer appear for this path.

Performance Impact

Slight build-time improvement: avoids constructing a full ResolutionResult for each upstream project during configuration of every Jar task.

Behavior Before vs After

Aspect Before After
How upstream runtime deps were derived Fully resolved incoming artifacts (ResolutionResult) of upstream project Uses declared first-level dependency metadata already collected
Cross-project configuration resolution Yes (unsafe) No
Impact on generated Class-Path manifest entries Functionally equivalent for first-level external deps; excludes any transitive-only jars that were previously (indirectly) included via resolved component graph

Why This Is Safe

The original logic only retained jars whose moduleVersion.id.name matched a declared dependency name captured in depMap. That effectively filtered the resolved component list back to the declared (first-level) external dependencies. Therefore the new approach—deriving jar names from declared dependencies—produces the same intended set for subtraction (upstreamDeps). Transitive-only artifacts (not directly declared) were not relied upon in the filtering logic.

For all changes:

  • Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?

  • Has your PR been rebased against the latest commit within the target branch (typically develop)?

  • Is your initial contribution a single, squashed commit?

  • Does gradlew build run cleanly?

  • Have you written or updated unit tests to verify your changes?

  • If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?

@JinwooHwang JinwooHwang requested a review from raboof September 14, 2025 23:25
@JinwooHwang JinwooHwang changed the title Cross-project runtimeClasspath resolution in geode-java.gradle Build Reliability Upgrade: Cross-project runtimeClasspath resolution in geode-java.gradle Sep 14, 2025
@JinwooHwang

JinwooHwang commented Sep 15, 2025

Copy link
Copy Markdown
Contributor Author

@raboof . All 17 checks have passed successfully. Let me know if there's anything you'd like to review together or if any concerns come up. Thank you so much for taking the time to review all the pull requests. Your contribution has been instrumental to our progress—we genuinely couldn’t have come this far without it.

@raboof raboof left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not much of a Gradle expert, so while this looks reasonable to me I'll ping @robbadler who seems more familiar with this part of the build

@raboof raboof requested a review from robbadler September 15, 2025 12:38
@JinwooHwang

Copy link
Copy Markdown
Contributor Author

Thank you for reviewing this, @raboof. I appreciate you proactively looping in @robbadler—looking forward to @robbadler 's insights so we can move this forward and wrap this up together.

@JinwooHwang

Copy link
Copy Markdown
Contributor Author

Hi @robbadler, I wanted to kindly follow up on my comment from yesterday. Your review would be greatly appreciated to help us finalize this. Looking forward to your insights.

@robbadler

Copy link
Copy Markdown
Contributor

Hi @JinwooHwang . Due to my current employment, I am not able to collaborate or contribute on this project at this time. I wish you luck on your work with Apache Geode.

@JinwooHwang

Copy link
Copy Markdown
Contributor Author

Hi @robbadler . Thanks for the heads-up. Totally understand and appreciate you letting us know. Wishing you all the best in your current role—may it bring both challenge and fulfillment. Hope our paths cross again when the timing aligns. Take care!

@sboorlagadda sboorlagadda self-requested a review September 26, 2025 16:18
.findAll { dep -> !(dep instanceof ProjectDependency) }
.each { dep ->
if (dep.hasProperty('version') && dep.version) {
upstreamDeps.add("${dep.name}-${dep.version}.jar")

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we have a fallback if any jar has no version in the filename? Or atleast log it if some depedency is skipped?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @sboorlagadda . That’s a sharp observation. If a jar lacks versioning in its filename, we should either have a fallback strategy or at minimum log it. Skipping silently could lead to subtle issues down the line, and visibility here is key. Let me get back to it and work on it. Thank you so much for your help, @sboorlagadda

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @sboorlagadda,
I've incorporated your suggestion to reflect the skipped dependency. Thanks for the thoughtful input—much appreciated! Please let me know if there's anything else I missed. Thank you.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you

@sboorlagadda sboorlagadda left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just see if we can log if some dependency is skipped or add a fallback?

@raboof raboof merged commit 87748ed into apache:develop Oct 24, 2025
17 checks passed
JinwooHwang added a commit to JinwooHwang/geode that referenced this pull request Oct 24, 2025
- Replace jakarta.activation-2.0.1.jar with angus-activation-2.0.0.jar
- Add istack-commons-runtime-4.1.1.jar (keeping 4.0.1 as both versions are in assembly)
- Upgrade txw2 from 3.0.2 to 4.0.2

This updates the expected assembly contents to match the actual build output
after merging PR apache#7925 which brought in Jakarta EE dependency updates.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants