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

Upgrade deps and use @CompileStatic everywhere #378

Merged
merged 6 commits into from Feb 22, 2020
Merged

Upgrade deps and use @CompileStatic everywhere #378

merged 6 commits into from Feb 22, 2020

Conversation

SUPERCILEX
Copy link
Collaborator

This PR:

Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>
@SUPERCILEX
Copy link
Collaborator Author

SUPERCILEX commented Feb 21, 2020

Plugin application perf improved by about 10x: old -> new.

Copy link
Owner

@ben-manes ben-manes left a comment

Choose a reason for hiding this comment

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

This looks great to me, thank you!

Anyone have any concerns with the Gradle 5 + JDK8 baseline requirements?

Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>
Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>
@ben-manes
Copy link
Owner

ben-manes commented Feb 21, 2020

Possibly we can condition the canBeResolved hack? This was added when ensuring compatibility with compile and 6.0 strongly encourages a transition to implementation and friends. In #334 there is a belief that us forcing it to be enabled breaks the newish Kotlin platform. If that would fix it, we could be gentle by not supporting compile in 6.2+ and assume those on earlier versions will migrate. In 7.0 they are being removed, so it was a compatibility issue when first introduced in #127.

@SUPERCILEX
Copy link
Collaborator Author

No, that doesn't seem to fix the issue (I reproed using Zac's app). It also broke the plugin because the resolved dependency set was empty.

@ben-manes
Copy link
Owner

Did you try skipping the configurations where canBeResolved is true, instead of us trying to force resolution? That's the fix I had in mind, as those user-defined configurations are deprecated. That could be done prior to any resolution logic, e.g. at

private Set<DependencyStatus> resolveProjects(Map<Project, Set<Configuration>> projectConfigs) {
projectConfigs.keySet().collect { proj ->
Set<Configuration> configurations = projectConfigs.get(proj)
Resolver resolver = new Resolver(proj, resolutionStrategy, checkConstraints)
configurations.collect { Configuration config ->
resolve(resolver, proj, config)
}.flatten() as Set<DependencyStatus>
}.flatten() as Set<DependencyStatus>
}

I was hoping that just ignoring those configuration types, like compile, would be okay in 6.2+ now and also fix their Kotlin problems. I just haven't had the time to help work on this plugin.

@SUPERCILEX
Copy link
Collaborator Author

Huh, I clearly don't understand how configuration works at all. I would have thought we'd want to skip canBeResolved = false. Gonna read up on this stuff and report back. 👍

@ben-manes
Copy link
Owner

Sorry, I meant skip canBeResolved = false like you said. :)

Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>
Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>
@SUPERCILEX
Copy link
Collaborator Author

Well yes, but no. 😁 Nothing I've tried fixes the issue where MPP deps can't be resolved. That said, only processing config.canBeResolved == false || config.canBeConsumed == true does actually fix that annoying issue where a ton of Failed to resolve ... logs are spewed. I had to special case annotation processors, but I feel like that quirk is worth it, especially since it also improves performance (~35% speedup on CatchUp: old -> new).

@SUPERCILEX
Copy link
Collaborator Author

From what I understand, this is what we're doing:

  • We process canBeResolved == false configurations.
  • That means things like implementation but not compileClasspath. This is actually good because it means we're only processing configurations users have explicitly set up.
  • When we resolve the configuration, we mark it as resolvable. How or why this works, I don't know.

@ben-manes
Copy link
Owner

We marked it as resolvable because most people used compile prior to Gradle 6. When it became unresolvable, then the plugin would have forced everyone to upgrade to the newest convention. Only in 7.x is the classic way finally gone, which means Gradle gave us major 4 releases to get there. In 6.x those are deprecated, we could assume everyone migrates, and don't need to hack the configuration anymore.

Would you prefer retaining the old hack for 5.x users and only applying your isUsefulConfiguration at a cutoff version, 6.2+? Or would you like to drop the hacks and assume everyone should have updated their builds to adopt the latest version of this plugin. I think either is a reasonable choice.

@SUPERCILEX
Copy link
Collaborator Author

It doesn't seem to be a hack though, we actually want configurations with canBeResolved == false and then we have to mark them as resolvable (or at least least when I comment out those two spots added in #127, the plugin doesn't find any of the dependencies).

@ben-manes
Copy link
Owner

ben-manes commented Feb 22, 2020

Oh okay. I suppose its been so many workarounds and Gradle versions that I no longer can tell how anything will work. 😄

When you're happy, feel free to merge. You are also welcome to cut a release:

  1. Increment version in build.gradle and commit
  2. Add Github release notes
  3. CI will see the new tag and deploy the archive.

@SUPERCILEX
Copy link
Collaborator Author

Dope, will do!

Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>
@SUPERCILEX SUPERCILEX merged commit 4ef6307 into ben-manes:master Feb 22, 2020
@SUPERCILEX SUPERCILEX deleted the perf branch February 22, 2020 04:18
@SUPERCILEX
Copy link
Collaborator Author

@ben-manes Do I have to do anything to make it show up on https://plugins.gradle.org?

@anuraaga
Copy link
Collaborator

anuraaga commented Feb 22, 2020

@SUPERCILEX Thanks for the help with this!

Just a strawman's proposal, would it be reasonable to go ahead and migrate the code to Java now that the baseline is Gradle 5? I enjoy helping to maintain this project, but do sometimes feel a little down when dealing with a Groovy gotcha. Not enough to not do it though, but I believe @ben-manes is also a Java person so maybe it's a good fit? If so, I'm happy to do the gruntwork

@ben-manes
Copy link
Owner

@SUPERCILEX Nope. It will sync, but is slow. We were early adopters of the plugin portal which at that time went through this sync phase, whereas perhaps now you can deploy to directly? All I know is it eventually works 😄

@anuraaga Groovy is certainly not my language and I used what Gradle expected at that time. I would be supportive of whatever current maintainers prefer (Java, Kotlin, etc). I'm no longer active except as a facilitator and will happily let those willing to do the work make the decisions.

@SUPERCILEX
Copy link
Collaborator Author

@anuraaga Yeah, I'd actually love to rewrite this plugin in Kotlin at some point—I just don't have the time 😅. I'd be willing to take a look at pull requests though (either Kotlin or Java) if somebody else has the time.

@ben-manes Lol, SGTM. I didn't know you could sync stuff to the plugin portal. And yeah, I deploy my plugins to the portal directly.

@bishiboosh
Copy link

Don't know if I will have the time to do it in one setting but I'm willing to try to rewrite the plugin in Kotlin

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.

Support Gradle 6.2 (due to changes in org.gradle.util.SingleMessageLogger) Gradle 6 deprecation warnings
6 participants