- Notifications
You must be signed in to change notification settings - Fork486
Progress eclipse cleanup#1587
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
blacelle commentedFeb 25, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@nedtwigg I had issues making it running in Cleanthat. Given the boilterplates around Eclipse integration, I feel preferable to drop Eclipse Cleanup from CleanThat, and get Cleanthat relying on Spotless. As it stands, this PR holds core integration, but not availability through build options (Gradle, Maven, etc). Do you prefer I open a separate PR for build-systems integrations ? (I will start with Eclipse). Or is it OK/preferable to have the whole thing in a single PR? |
Single PR is fine. |
This would help#1379 (comment) |
Any idea@nedtwigg ? |
That whole project structure is about to be nuked. Everything in the |
| * JDT core manipulation required for clean-up base interfaces and import sorting | ||
| * It depends on JDT core, which is required for formatting. | ||
| */ | ||
| /**compile("org.eclipse.jdt:org.eclipse.jdt.core.manipulation:${VER_ECLIPSE_JDT_CORE_MANIPULATION}") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@nedtwigg This was in the old eclipse-jdt build.gradle. I suppose it has to be converted into things like:
public static EquoBasedStepBuilder createBuilder(Provisioner provisioner) {return new EquoBasedStepBuilder(NAME, provisioner, EclipseJdtCleanUpStep::apply) {@Overrideprotected P2Model model(String version) {var model = new P2Model();addPlatformRepo(model, version);model.getInstall().add("org.eclipse.jdt.core");return model;}There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
That is correct.
| privatefinalMap<String,String>defaultOptions; | ||
| protectedEclipseJdtCoreManipulation()throwsException { | ||
| if (SpotlessEclipseFramework.setup( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I need to check the migration of EclipseJdtFormatterStepImpl to see how this was migrated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
ThisSpotlessEclipseFramework stuff is tricky. Depending on what version of Eclipse, and what methods you are calling, it might not matter if any of the OSGi stuff has been initialized at all. Or you might need all of it initialized.
For the current Eclipse JDT and CDT formatters, nothing is initialized at all, and that works fine. For Groovy we needed to start the full Solstice OSGi thing. If you are getting NPE or "blah not initialized" errors, I would copy this into your static initializer as a template.
Lines 52 to 70 inbc21f83
| publicclassGrEclipseFormatterStepImpl { | |
| static { | |
| NestedJars.setToWarnOnly(); | |
| NestedJars.onClassPath().confirmAllNestedJarsArePresentOnClasspath(CacheLocations.nestedJars()); | |
| try { | |
| varsolstice =Solstice.findBundlesOnClasspath(); | |
| solstice.warnAndModifyManifestsToFix(); | |
| varprops =Map.of("osgi.nl","en_US", | |
| Constants.FRAMEWORK_STORAGE_CLEAN,Constants.FRAMEWORK_STORAGE_CLEAN_ONFIRSTINIT, | |
| EquinoxLocations.PROP_INSTANCE_AREA,Files.createTempDirectory("spotless-groovy").toAbsolutePath().toString()); | |
| solstice.openShim(props); | |
| ShimIdeBootstrapServices.apply(props,solstice.getContext()); | |
| solstice.start("org.apache.felix.scr"); | |
| solstice.startAllWithLazy(false); | |
| solstice.start("org.codehaus.groovy.eclipse.core"); | |
| }catch (IOExceptione) { | |
| thrownewRuntimeException(e); | |
| } | |
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thanks for the insights. For now, I'm feeling a bit overwhelmed here. I'll take some time to get through it.
| config.put("invalid.key","some.value"); | ||
| }); | ||
| cleanUpTest("","",config -> { | ||
| config.put(JavaCore.COMPILER_SOURCE,"-42"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
JavaCore is unknown here, while it is known in thejdt.compile module. How should I manage additional dependencies in test classes in lib-extra?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@nedtwigg I would need a hint here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The build does not currently have a way to run tests against the Eclipse classpath. We would have to add something into this block
spotless/lib-extra/build.gradle
Lines 47 to 56 inbc21f83
| for (needsP2inNEEDS_P2_DEPS) { | |
| sourceSets.register(needsP2) { | |
| compileClasspath+= sourceSets.main.output | |
| runtimeClasspath+= sourceSets.main.output | |
| java {} | |
| } | |
| dependencies { | |
| add("${needsP2}CompileOnly","dev.equo.ide:solstice:${VER_SOLSTICE}") | |
| } | |
| } |
Along these lines
tasks.register("${needsP2}Test", Test) { // setup classpath somehow}...into 'jdtCompileOnly', { ... }into 'jdtTestImplementation', { ... }There would be some copy pasting going on, if this got implemented then there'd be no copy-pasting necessary
This is still quite a long way to go.
|
Uh oh!
There was an error while loading.Please reload this page.
This revives the interest in Eclipse JDT CleanUp.
I resynced the branch with Spotless main.