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

Fix plugin not working if module has a lot of dependencies #50

Merged
merged 5 commits into from
Sep 13, 2024

Conversation

Mexator
Copy link
Contributor

@Mexator Mexator commented Aug 27, 2024

In my project I have ~1000 gradle projects. I use focus to ease editing module with integration tests. It does not depend on all module, but it still have ~700 dependencies. I receive the following error:

A large stacktrace
...
Caused by: java.io.IOException: Failed to process the entry '_BuildScript_.class' from '~/.gradle/caches/8.7/groovy-dsl/9d7e202a95c600b4e91905ecc929318a-48a2fdac-7ee7-43b0-b7c7-bba0a26b072f/classes/dsl'
	at org.gradle.internal.classpath.transforms.BaseClasspathElementTransform.visitEntry(BaseClasspathElementTransform.java:93)
	at org.gradle.internal.classpath.transforms.BaseClasspathElementTransform.lambda$visitEntries$1(BaseClasspathElementTransform.java:78)
	at org.gradle.internal.classpath.ClasspathWalker.visitFile(ClasspathWalker.java:82)
	at org.gradle.internal.classpath.ClasspathWalker.visitDir(ClasspathWalker.java:74)
	at org.gradle.internal.classpath.ClasspathWalker.visitDirectoryContents(ClasspathWalker.java:62)
	at org.gradle.internal.classpath.ClasspathWalker.visit(ClasspathWalker.java:57)
	at org.gradle.internal.classpath.transforms.BaseClasspathElementTransform.visitEntries(BaseClasspathElementTransform.java:77)
	at org.gradle.internal.classpath.transforms.BaseClasspathElementTransform.lambda$transform$0(BaseClasspathElementTransform.java:68)
	at org.gradle.internal.classpath.InPlaceClasspathBuilder.buildJar(InPlaceClasspathBuilder.java:52)
	at org.gradle.internal.classpath.InPlaceClasspathBuilder.jar(InPlaceClasspathBuilder.java:42)
	... 254 more
Caused by: org.objectweb.asm.MethodTooLargeException: Method too large: _BuildScript_.run ()Ljava/lang/Object;
	at org.objectweb.asm.MethodWriter.computeMethodInfoSize(MethodWriter.java:2088)
	at org.objectweb.asm.ClassWriter.toByteArray(ClassWriter.java:512)
	at org.gradle.internal.classpath.transforms.BaseClasspathElementTransform.processClassFile(BaseClasspathElementTransform.java:111)
	at org.gradle.internal.classpath.transforms.BaseClasspathElementTransform.visitEntry(BaseClasspathElementTransform.java:86)
	... 263 more

This is caused by focus behavior: it generates a huge buildscript (~1400 lines). Java can't process methods such big.
To fix this I made focus generate a CSV with list of modules that needs to be included and a small single-loop buildscript that parses that CSV.

Also I added configuration cache support for createFocusSetings task.

@CLAassistant
Copy link

CLAassistant commented Aug 27, 2024

CLA assistant check
All committers have signed the CLA.

@Mexator
Copy link
Contributor Author

Mexator commented Sep 9, 2024

Hey, @rharter, I want to bring your attention to this PR, since you may have missed it.
I assume you are a maintainer. Correct me if I'm wrong

Copy link
Collaborator

@rharter rharter left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution. I left some comments and questions.

@@ -51,17 +67,31 @@ class FocusPluginTest {
}

@Test
fun singleQuotePath() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of replacing this test, can you add a new one? The single-quote test was intentionally introduced as part of resolving #25.

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 restored the original test with some changes, take a look

projectPath.set(project.path)
// Collecting dependencies info in the configuration phase.
// Gradle disallows accessing project in execution phase.
projectDeps.set(project.collectDependencies())
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this end up forcing dependency resolution at configuration time? When I originally wrote this it was explicitly excluded from configuration caching and deferred dependency resolution to action-time to avoid exploding the configuration phase of a project due to dependency resolution.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, yeah. I can't figure out how to support configuration cache and resolve dependencies at execution phase.
I tried accessing ConfigurationContainer but for some reason it's empty.

To reproduce this issue I add gradle.properties:

org.gradle.configuration-cache=true
org.gradle.configuration-cache.problems=warn

I reverted configuration cache changes.

@Mexator Mexator requested a review from rharter September 11, 2024 19:16
Copy link
Collaborator

@rharter rharter left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks!

@rharter
Copy link
Collaborator

rharter commented Sep 11, 2024

Looks like this just needs the apiDump task run to update the API. The updates look fine to me (they're just adds), and I can do it tomorrow during work if you don't get to it first.

@rharter rharter merged commit 279bd3d into dropbox:main Sep 13, 2024
2 of 3 checks passed
@Mexator
Copy link
Contributor Author

Mexator commented Oct 17, 2024

Hi, @rharter,

Could you please release a version of the plugin with this fix? I'm using the build of this plugin from maven-local, and it's a bit frustrating.

@rharter
Copy link
Collaborator

rharter commented Oct 17, 2024

Done, as 0.7.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants