-
Notifications
You must be signed in to change notification settings - Fork 22
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
Conversation
Hey, @rharter, I want to bring your attention to this PR, since you may have missed it. |
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 contribution. I left some comments and questions.
focus-gradle-plugin/src/main/kotlin/com/dropbox/focus/CreateFocusSettingsTask.kt
Outdated
Show resolved
Hide resolved
@@ -51,17 +67,31 @@ class FocusPluginTest { | |||
} | |||
|
|||
@Test | |||
fun singleQuotePath() { |
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.
Instead of replacing this test, can you add a new one? The single-quote test was intentionally introduced as part of resolving #25.
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 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()) |
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.
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.
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.
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.
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.
Looks great! Thanks!
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. |
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. |
Done, as 0.7.0. |
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
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.