Add Eclipse JDT embedded lockfiles, upgrade formatter version to 4.40 - #3000
Add Eclipse JDT embedded lockfiles, upgrade formatter version to 4.40#3000scordio wants to merge 10 commits into
Conversation
I tried that locally, but it seems I have some cache configuration errors. Logs |
|
Excellent start! The lockfiles you have right now are keyed in only on the "root" dependency - The lockfiles need to set the version of all transitive dependencies, and then the maven configuration needs to be initialized with "downloadTransitives=false" and then explicitly seeded with all the needed transitives and their versions. The reason for this is that Eclipse tends to specify transitives in a wide range, see for examples: Now, it looks like Eclipse may have changed its policy for recent releases. I just looked at the metadata for 4.40, and I was surprised to see that it specified exact versions rather than broad ranges. So I think there's two options forward:
Doing A is easier. And it has the nice benefit that after A has been merged, you can always go back and do B and then we could turn it on for all versions at any time. Happy to move forward with either. |
# Conflicts: # CHANGES.md # plugin-gradle/CHANGES.md # plugin-maven/CHANGES.md
6331dcc to
045ea2a
Compare
The first version on Maven Central having exact versions seems to be 3.32.0. I think the discussion about having exact versions happened at: and the change that actually did it is: merged in October 2022, before 3.32.0 was published in December. |
I solved my local issues by pinning the Eclipse version to 4.40 in diff --git a/gradle/spotless.gradle b/gradle/spotless.gradle
--- a/gradle/spotless.gradle
+++ b/gradle/spotless.gradle
@@ -6,8 +6,8 @@
ratchetFrom 'origin/main'
bumpThisNumberIfACustomStepChanges(1)
licenseHeaderFile rootProject.file('gradle/spotless.license')
importOrderFile rootProject.file('gradle/spotless.importorder')
- eclipse().configFile rootProject.file('gradle/spotless.eclipseformat.xml')
+ eclipse('4.40').configFile rootProject.file('gradle/spotless.eclipseformat.xml')
trimTrailingWhitespace()
removeUnusedImports()
formatAnnotations()
@@ -17,7 +17,7 @@
}
groovyGradle {
target '*.gradle', 'gradle/*.gradle'
- greclipse().configFile rootProject.files('gradle/spotless.eclipseformat.xml', 'gradle/spotless.groovyformat.prefs')
+ greclipse('4.40').configFile rootProject.files('gradle/spotless.eclipseformat.xml', 'gradle/spotless.groovyformat.prefs')
}
format 'dotfiles', {
target '.gitignore', '.gitattributes', '.editorconfig' |
I initially implemented A with 8bb5194, but then spent a bit more time and also implemented B with 71c40eb 🙃 Happy to know what you think! |
…se-jdt-lockfiles # Conflicts: # CHANGES.md # plugin-gradle/CHANGES.md # plugin-maven/CHANGES.md
Eclipse JDT now resolves from an embedded lockfile, so it never runs a P2 query. Two tests still asserted the P2 path: - FormatterStepFactoryTest's eclipse cache-directory cases asserted the P2 provisioner received the cache directory. Replaced with a test that the lockfile path is taken (Maven coordinates resolved, P2 never queried). grEclipse and eclipseCdt have no lockfile and still cover the P2 cache directory wiring. - predeclareFailsWhenEclipseFormatterNotPredeclared expected the P2 'not predeclared' message; an un-predeclared subproject now fails on the Maven coordinates instead. greclipse still covers the P2 message. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…se-jdt-lockfiles # Conflicts: # plugin-maven/CHANGES.md
The lockfile set was six sampled versions, chosen to span Eclipse's switch
from OSGi ranges to concrete versions in its Maven metadata (releng#135,
effective 4.26). That made the P2 fallback depend on which release a user
happened to pin: 4.30 silently resolved differently from 4.39, with a
different predeclare error and cacheDirectory going unused on one path but
not the other.
Ship 4.9 through the default version contiguously instead. The range/exact
distinction does not need to factor in -- Solstice resolves concrete
coordinates under either style, so a lockfile can be produced for any
target. Regenerating the six existing lockfiles reproduces them byte for
byte.
- EclipseJdtLockfileMetadataTool derives its targets from the default
version rather than a hand-maintained list, and Verify now exits non-zero
so it can gate.
- Gradle entrypoints :lib-extra:{verify,update}EclipseJdtLockfiles. Verify
queries the Eclipse update sites, so it runs in scheduled-cleanup rather
than per-PR; gaps and strays are caught offline by a new test.
- EquoBasedStepBuilder now derives the lockfile path from the formatter
name, the same layout EclipseBasedStepBuilder already uses, so cdt and
groovy pick up lockfiles whenever theirs are checked in. Both continue to
fall back to P2 today.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Excellent! This looks good to me now - you want any changes before it gets merged and released? |
2 participants
This PR introduces lockfiles for the Eclipse JDT formatter, falling back to today's behavior via P2 resolution if an embedded lockfile for the selected version doesn't exist.
In addition, the default JDT version is now upgraded to the latest available (4.40), and embedded lockfiles have been added for versions 4.40, 4.39, 4.11, and 4.9. The rationale for covering the last two is only because those versions appeared in test code, but please let me know what the proper version range should be, and I'll happily adjust this PR and/or introduce missing lockfiles.
To support maintenance and future lockfile generation, I added a new
EclipseJdtLockfileMetadataToolamong the test classes. This class has twomainentry points that can be manually invoked to either verify or update the existing lockfiles by resolvingorg.eclipse.jdt.coreversions directly from the metadata available athttps://download.eclipse.org/eclipse/updates/. If you think there is a better approach to achieve the same, please let me know, and I'd be happy to update my changes.Disclaimer: this is my first Spotless contribution and I wasn't familiar with its codebase and internal architecture. For this reason, I used GitHub Copilot to navigate through it, brainstorm on a possible solution, and implement the self-contained
EclipseJdtLockfileMetadataToolclass to make lockfile maintenance easier. Nevertheless, I went through and updated pretty much everything that Copilot generated.Please DO NOT FORCE PUSH. Don't worry about messy history, it's easier to do code review if we can tell what happened after the review, and force pushing breaks that.
Please make sure that your PR allows edits from maintainers. Sometimes it's faster for us to just fix something than it is to describe how to fix it.
After creating the PR, please add a commit that adds a bullet-point under the
[Unreleased]section of CHANGES.md, plugin-gradle/CHANGES.md, and plugin-maven/CHANGES.md which includes:If your change only affects a build plugin, and not the lib, then you only need to update the
plugin-foo/CHANGES.mdfor that plugin.If your change affects lib in an end-user-visible way (fixing a bug, updating a version) then you need to update
CHANGES.mdfor both the lib and all build plugins. Users of a build plugin shouldn't have to refer to lib to see changes that affect them.This makes it easier for the maintainers to quickly release your changes :)