Skip to content
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

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

blacelle
Copy link
Contributor

@blacelle blacelle commented Feb 25, 2023

This revives the interest in Eclipse JDT CleanUp.

  1. The implementation is rooted in main...issue_292, but I would pursue the work in a forked repository.
  2. This is concurrent to my work in CleanThat, integrating various refactoring/cleanups engines (Progress with Eclipse CleanUp as Refactorer solven-eu/cleanthat#423)
  3. Merging would probably waits the rework of Eclipse integration in Spotless Use Equo Solstice to calculate and download eclipse dependencies #1524
  4. This is a direct answer to Add support for Eclipse Clean Up #292
  5. Unittests in _ext/eclipse-jdt are not available to myself from Intellij (they are grayed, like in case of classpath issue, hence not detected as source/unitTests). Is there a tips to get them working? (IntelliJ)
    image

I resynced the branch with Spotless main.

@blacelle
Copy link
Contributor Author

blacelle commented Feb 25, 2023

@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?

@nedtwigg
Copy link
Member

Single PR is fine.

@blacelle
Copy link
Contributor Author

blacelle commented Mar 9, 2023

This would help #1379 (comment)

@blacelle
Copy link
Contributor Author

Unittests in _ext/eclipse-jdt are not available to myself from Intellij (they are grayed, like in case of classpath issue, hence not detected as source/unitTests). Is there a tips to get them working? (IntelliJ)

Any idea @nedtwigg ?

@nedtwigg
Copy link
Member

That whole project structure is about to be nuked. Everything in the _ext is becoming just a plain old sourceSet of lib-extra in #1524 which I will merge later today.

* 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}") {
Copy link
Contributor Author

@blacelle blacelle Mar 13, 2023

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;
		}

Copy link
Member

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(
Copy link
Contributor Author

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.

Copy link
Member

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.

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);
}
}

Copy link
Contributor Author

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");
Copy link
Contributor Author

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?

Copy link
Contributor Author

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.

Copy link
Member

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

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

@blacelle
Copy link
Contributor Author

This is still quite a long way to go.

  1. Enable customization/selection of CleanUp rules
  2. Migration to updated Eclipse. (e.g. around Indexer.getInstance().enableAutomaticIndexing(false);)
  3. There was no unitTest in the initial work for cleanup (but there was for organizeImports). Hence, I'm not confident the minimal work was working fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-archive PRs which are still valid but have gotten stuck for some reason
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants