-
Notifications
You must be signed in to change notification settings - Fork 460
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Progress eclipse cleanup #1587
base: main
Are you sure you want to change the base?
Progress eclipse cleanup #1587
Conversation
@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) {
@Override
protected 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.
private final Map<String, String> defaultOptions; | ||
|
||
protected EclipseJdtCoreManipulation() throws Exception { | ||
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.
This SpotlessEclipseFramework
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 in bc21f83
public class GrEclipseFormatterStepImpl { | |
static { | |
NestedJars.setToWarnOnly(); | |
NestedJars.onClassPath().confirmAllNestedJarsArePresentOnClasspath(CacheLocations.nestedJars()); | |
try { | |
var solstice = Solstice.findBundlesOnClasspath(); | |
solstice.warnAndModifyManifestsToFix(); | |
var props = 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 (IOException e) { | |
throw new RuntimeException(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 the jdt.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 in bc21f83
for (needsP2 in NEEDS_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.
|
This revives the interest in Eclipse JDT CleanUp.
I resynced the branch with Spotless main.