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.

Sign up for GitHub

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

Do not add Kotlin and Groovy sources from build/ to LST #322

Merged
merged 2 commits into from
Aug 18, 2024
Merged

Do not add Kotlin and Groovy sources from build/ to LST #322

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7591770
Exclude build-generated-sources too
timtebeek Aug 15, 2024
be668d2
Consistently skip source paths that start with BuildDirectory
timtebeek Aug 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 14 additions & 8 deletions plugin/src/main/java/org/openrewrite/gradle/isolated/DefaultProjectParser.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,7 @@ static Path repositoryRoot(Project project) {
}

private static final Map<Path, GitProvenance> REPO_ROOT_TO_PROVENANCE = new HashMap<>();

private @Nullable GitProvenance gitProvenance(Path baseDir, @Nullable BuildEnvironment buildEnvironment) {
try {
// Computing git provenance can be expensive for repositories with many commits, ensure we do it only once per build
Expand Down Expand Up @@ -660,13 +661,14 @@ public Stream<SourceFile> parse(Project subproject, Set<Path> alreadyParsed, Exe
}

if (subproject.getPlugins().hasPlugin("org.jetbrains.kotlin.gradle.dsl.KotlinMultiplatformExtension") ||
subproject.getExtensions().findByName("kotlin") != null && subproject.getExtensions().getByName("kotlin").getClass()
.getCanonicalName().startsWith("org.jetbrains.kotlin.gradle.dsl.KotlinMultiplatformExtension")) {
subproject.getExtensions().findByName("kotlin") != null && subproject.getExtensions().getByName("kotlin").getClass()
.getCanonicalName().startsWith("org.jetbrains.kotlin.gradle.dsl.KotlinMultiplatformExtension")) {
sourceFileStream = sourceFileStream.concat(parseMultiplatformKotlinProject(subproject, exclusions, alreadyParsed, ctx));
}

Charset sourceCharset = Charset.forName(System.getProperty("file.encoding", "UTF-8"));

Path buildDirPath = baseDir.relativize(subproject.getLayout().getBuildDirectory().get().getAsFile().toPath());
for (SourceSet sourceSet : sourceSets) {
Stream<SourceFile> sourceSetSourceFiles = Stream.of();
int sourceSetSize = 0;
Expand Down Expand Up @@ -737,7 +739,7 @@ public Stream<SourceFile> parse(Project subproject, Set<Path> alreadyParsed, Exe
.flatMap(jp -> jp.parse(javaPaths, baseDir, ctx))
.map(cu -> {
if (isExcluded(exclusions, cu.getSourcePath()) ||
cu.getSourcePath().startsWith(baseDir.relativize(subproject.getLayout().getBuildDirectory().get().getAsFile().toPath()))) {
cu.getSourcePath().startsWith(buildDirPath)) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to even be parsing these files at all with OpenRewrite?

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 thought we did so for the type attribution; that's at least the comment over on the Maven plugin. Hadn't thought to change that here just yet. Just excluding them after they are parsed to prevent them ending up in the LST themselves.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we have so far, yeah, but we also trigger the compile phases in Gradle as well prior to the rewrite tasks, so technically we could put the source set's own output classes onto the parser's classpath to still get the necessary type attribution and bypass parsing the generated types.

Just a thought though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could indeed be a nice improvement to stop ourselves from parsing source files whose LSTs we'll discard anyway, or perhaps even having two sources of the same type information; compiled vs source.

return null;
}
return cu;
Expand All @@ -752,8 +754,8 @@ public Stream<SourceFile> parse(Project subproject, Set<Path> alreadyParsed, Exe
if (subproject.getPlugins().hasPlugin("org.jetbrains.kotlin.jvm")) {
String excludedProtosPath = subproject.getProjectDir().getPath() + "/protos/build/generated";
List<Path> kotlinPaths = unparsedSources.stream()
.filter(it -> !it.toString().startsWith(excludedProtosPath))
.filter(it -> it.toString().endsWith(".kt"))
.filter(it -> !it.toString().startsWith(excludedProtosPath))
.collect(toList());

if (!kotlinPaths.isEmpty()) {
Expand All @@ -768,7 +770,8 @@ public Stream<SourceFile> parse(Project subproject, Set<Path> alreadyParsed, Exe
.map(Supplier::get)
.flatMap(kp -> kp.parse(kotlinPaths, baseDir, ctx))
.map(cu -> {
if (isExcluded(exclusions, cu.getSourcePath())) {
if (isExcluded(exclusions, cu.getSourcePath()) ||
cu.getSourcePath().startsWith(buildDirPath)) {
return null;
}
return cu;
Expand Down Expand Up @@ -805,7 +808,8 @@ public Stream<SourceFile> parse(Project subproject, Set<Path> alreadyParsed, Exe
.map(Supplier::get)
.flatMap(gp -> gp.parse(groovyPaths, baseDir, ctx))
.map(cu -> {
if (isExcluded(exclusions, cu.getSourcePath())) {
if (isExcluded(exclusions, cu.getSourcePath()) ||
cu.getSourcePath().startsWith(buildDirPath)) {
return null;
}
return cu;
Expand Down Expand Up @@ -1073,6 +1077,7 @@ private SourceFileStream parseMultiplatformKotlinProject(Project subproject, Col
return sourceFileStream;
}

Path buildDirPath = baseDir.relativize(subproject.getLayout().getBuildDirectory().get().getAsFile().toPath());
for (String sourceSetName : sourceSetNames) {
try {
Object sourceSet = sourceSets.getClass().getMethod("getByName", String.class)
Expand Down Expand Up @@ -1131,7 +1136,8 @@ private SourceFileStream parseMultiplatformKotlinProject(Project subproject, Col
Stream<SourceFile> cus = kp.parse(kotlinPaths, baseDir, ctx);
alreadyParsed.addAll(kotlinPaths);
cus = cus.map(cu -> {
if (isExcluded(exclusions, cu.getSourcePath())) {
if (isExcluded(exclusions, cu.getSourcePath()) ||
cu.getSourcePath().startsWith(buildDirPath)) {
return null;
}
return cu;
Expand All @@ -1151,7 +1157,7 @@ private SourceFileStream parseMultiplatformKotlinProject(Project subproject, Col
}

private SourceFile logParseErrors(SourceFile source) {
source.getMarkers().findFirst(ParseExceptionResult.class).ifPresent(e -> {
source.getMarkers().findFirst(ParseExceptionResult.class).ifPresent(e -> {
if (firstWarningLogged.compareAndSet(false, true)) {
logger.warn("There were problems parsing some source files, run with --info to see full stack traces");
}
Expand Down
Loading