Add Maven plugin integration for ExpandWildcardImportsStep#3
Add Maven plugin integration for ExpandWildcardImportsStep#3
Conversation
Co-authored-by: shitikanth <544054+shitikanth@users.noreply.github.com>
Co-authored-by: shitikanth <544054+shitikanth@users.noreply.github.com>
…le directories Co-authored-by: shitikanth <544054+shitikanth@users.noreply.github.com>
Co-authored-by: shitikanth <544054+shitikanth@users.noreply.github.com>
|
Some issues I could see when trying it locally (I'm not a Maven expert either):
|
|
@copilot add a test with multi-module maven project |
Co-authored-by: shitikanth <544054+shitikanth@users.noreply.github.com>
Added multi-module test in commit 0e5abc2. The test creates a project with:
|
|
@copilot can you run the test to check if it passes? |
|
@awa-xima Thanks for the thoughful comments. Looks like in general we would have to use the |
|
Just as a note, we are using spotless in a way that does not work with this approach at all: We have a large multi-module project with 100+ sub modules. Running Maven / spotless on each module would take some time. So we just run Maven on the parent pom and have it format all files in our project. That's fairly fast. But it means that the classpath is not known to Maven. So I think we'll just use the ban wildcard imports step and rely on people configuring their IDE properly. @shitikanth You're welcome, I'd also like to see it work for Maven. From what little I know from Maven plugins, it's fairly complicated to get it right. Which But with As for multi-module projects, the behavior is also "interesting":
For reference, my first approach with the dependency resolver (similar to maven-dependency-plugin) public abstract class AbstractSpotlessMojo extends AbstractMojo {
// ...
private Set<JavaSource> resolveClasspathElements() {
final var classpathElements = new HashSet<JavaSource>();
try {
var artifact = project.getArtifact();
var aetherArtifact = new DefaultArtifact(artifact.getGroupId(), artifact.getArtifactId(), artifact.getClassifier(), artifact.getType(), artifact.getVersion(), null, artifact.getFile());
var collectRequest = new CollectRequest();
collectRequest.setRepositories(repositories);
collectRequest.setRoot(new Dependency(aetherArtifact, null));
var request = new DependencyRequest(collectRequest, null);
var resolved = repositorySystem.resolveDependencies(repositorySystemSession, request);
project.getCompileSourceRoots().stream().map(File::new).map(file -> new JavaSource(JavaSourceType.SOURCE_CODE, file)).forEach(classpathElements::add);
project.getTestCompileSourceRoots().stream().map(File::new).map(file -> new JavaSource(JavaSourceType.SOURCE_CODE, file)).forEach(classpathElements::add);
resolved.getArtifactResults().stream().map(ArtifactResult::getArtifact).filter(Objects::nonNull).map(Artifact::getFile).filter(Objects::nonNull).map(e -> new JavaSource(JavaSourceType.BYTE_CODE, e)).forEach(classpathElements::add);
return Set.copyOf(classpathElements);
} catch (Exception e) {
throw new PluginException("Unable to resolve project artifacts", e);
}
} |
|
For very large projects, I don't think it is practical (or even necessary) to check the formatting of every single source file. It makes much more sense to run it only on the files changed in current PR (using I recently contributed a PR that significantly improves the performance of I don't think it is a good idea to bypass the build system completely and recreate the dependency resolution logic as part of the plugin.
|
I disagree. It should not take significantly more time to have maven run spotless on each module separately rather than only on the root project. Overhead introduced in initializing the plugin for each sub-module should be small enough that it shouldn't matter. It is a performance bug if that is not the case. |
That's my prediction. But I'm willing to give it a try, which is why I'd also like to see it implemented for Maven ; ) Spotless already has an index/cache file . Most of the time, we don't change many files in any particular commit, so spotless doesn't do much. Running
I can only find it in the Maven 4.x branch of the maven-compiler-plugin, not in the Maven 3.x branch. |
|
Are you sure that approach is actually buying you any real performance benefits? I just tried running (I presume this is sort of what you are doing to apply changes to all source files from the root project). With the cache primed, this approach takes ~8s, whereas vanilla |
Actually no. It just was fast enough so far, so I didn't try another approach. I just tried it, running it once on every module. With the cache enabled it's pretty much just as fast : ) So thanks for bringing it up again, I might not have tried otherwise. (The unfortunate part is that it looks like people want to introduce lombok and javacompiler understandably can't handle that, but that's another story.) |
ExpandWildcardImportsStepwas already implemented in lib and Gradle plugin but not available in Maven plugin. The step requires aCollection<File> typeSolverClasspathcontaining project sources and dependencies for type resolution.Changes
Configuration chain extended to pass
MavenProject:MavenProjectfield toFormatterConfigandFormatterStepConfigFormatterFactoryandAbstractSpotlessMojoto propagate project referenceCreated
ExpandWildcardImportsFormatterStepFactory:getCompileSourceRoots()andgetTestCompileSourceRoots()typeSolverClasspathand delegates to existingExpandWildcardImportsStep.create()Integration:
addExpandWildcardImports()method toJavaformatterUsage
CODE_BLOCK_0_0
Transforms:CODE_BLOCK_0_1
Into:CODE_BLOCK_0_2
Testing
The implementation is validated with comprehensive integration tests:
commonandappmodules whereappdepends oncommon, ensuring that wildcard imports are correctly expanded across module boundaries and inter-module dependencies are properly resolvedWarning
Firewall rules blocked me from connecting to one or more addresses (expand for details)
I tried to connect to the following addresses, but was blocked by firewall rules:
archive.eclipse.org/usr/lib/jvm/temurin-17-jdk-amd64/bin/java /usr/lib/jvm/temurin-17-jdk-amd64/bin/java --add-opens=java.base/java.lang=ALL-UNNAMED --add-opens=java.base/java.lang.invoke=ALL-UNNAMED --add-opens=java.base/java.util=ALL-UNNAMED --add-opens=java.prefs/java.util.prefs=ALL-UNNAMED --add-exports=jdk.compiler/com.sun.tools.javac.api=ALL-UNNAMED --add-exports=jdk.compiler/com.sun.tools.javac.util=ALL-UNNAMED --add-opens=java.base/java.util=ALL-UNNAMED --add-opens=java.prefs/java.util.prefs=ALL-UNNAMED --add-opens=java.base/java.nio.charset=ALL-UNNAMED --add-opens=java.base/java.net=ALL-UNNAMED --add-opens=java.base/java.util.concurrent=ALL-UNNAMED --add-opens=java.base/java.util.concurrent.atomic=ALL-UNNAMED --add-opens=java.xml/javax.xml.namespace=ALL-UNNAMED --add-opens=java.base/java.time=ALL-UNNAMED -XX:MaxMetaspaceSize=1024m -Xmx4g -Dfile.encoding=UTF-8 -Duser.country -Duser.language=en(dns block)java.sun.com/opt/hostedtoolcache/CodeQL/2.23.9/x64/codeql/tools/linux64/java/bin/java /opt/hostedtoolcache/CodeQL/2.23.9/x64/codeql/tools/linux64/java/bin/java -jar /opt/hostedtoolcache/CodeQL/2.23.9/x64/codeql/xml/tools/xml-extractor.jar --fileList=/home/REDACTED/work/spotless/.codeql-scratch/dbs/java/working/files-to-index2995331916609997450.list --sourceArchiveDir=/home/REDACTED/work/spotless/.codeql-scratch/dbs/java/src --outputDir=/home/REDACTED/work/spotless/.codeql-scratch/dbs/java/trap/java(dns block)scans-in.gradle.com/usr/lib/jvm/temurin-17-jdk-amd64/bin/java /usr/lib/jvm/temurin-17-jdk-amd64/bin/java --add-opens=java.base/java.lang=ALL-UNNAMED --add-opens=java.base/java.lang.invoke=ALL-UNNAMED --add-opens=java.base/java.util=ALL-UNNAMED --add-opens=java.prefs/java.util.prefs=ALL-UNNAMED --add-exports=jdk.compiler/com.sun.tools.javac.api=ALL-UNNAMED --add-exports=jdk.compiler/com.sun.tools.javac.util=ALL-UNNAMED --add-opens=java.base/java.util=ALL-UNNAMED --add-opens=java.prefs/java.util.prefs=ALL-UNNAMED --add-opens=java.base/java.nio.charset=ALL-UNNAMED --add-opens=java.base/java.net=ALL-UNNAMED --add-opens=java.base/java.util.concurrent=ALL-UNNAMED --add-opens=java.base/java.util.concurrent.atomic=ALL-UNNAMED --add-opens=java.xml/javax.xml.namespace=ALL-UNNAMED --add-opens=java.base/java.time=ALL-UNNAMED -XX:MaxMetaspaceSize=1024m -Xmx4g -Dfile.encoding=UTF-8 -Duser.country -Duser.language=en(dns block)>
> If you need me to access, download, or install something from one of these locations, you can either:
>
> - Configure Actions setup steps to set up my environment, which run before the firewall is enabled
> - Add the appropriate URLs or hosts to the custom allowlist in this repository's Copilot coding agent settings (admins only)
>
>
typeSolverClasspath to pass to java parser, so we need to create that using information from maven project and pass it to the the step
Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.