-
Notifications
You must be signed in to change notification settings - Fork 50
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
Use variant-aware dependency management #132
Conversation
src/main/groovy/org/jenkinsci/gradle/plugins/jpi/JpiExtension.groovy
Outdated
Show resolved
Hide resolved
src/main/groovy/org/jenkinsci/gradle/plugins/jpi/JpiVariantRule.groovy
Outdated
Show resolved
Hide resolved
src/main/groovy/org/jenkinsci/gradle/plugins/jpi/LicenseTask.groovy
Outdated
Show resolved
Hide resolved
Thanks so much for this @jjohannes! 🎉 At first glance it looks like there are some significant changes that aren’t backwards compatible. I’d prefer to deprecate the old components and bridge to the new ones in order to simplify upgrading and migrating for consumers. I’ll take me some time to look through all the changes here, but wanted to make sure you knew I saw the PR. |
Thanks @sghill! I have also discussed some more things today with @wolfs and found some issues. So I am still working on this. And yes there are some fundamental changes (for the better). But I agree that we have to discuss compatibility. Once this is in a state where we think it is good on its own, we can have a look at compatibility to assess what we want and how hard it is to do. |
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.
Some more feedback.
src/main/groovy/org/jenkinsci/gradle/plugins/jpi/DependencyAnalysis.groovy
Outdated
Show resolved
Hide resolved
src/main/groovy/org/jenkinsci/gradle/plugins/jpi/DependencyAnalysis.groovy
Show resolved
Hide resolved
src/main/groovy/org/jenkinsci/gradle/plugins/jpi/JpiPlugin.groovy
Outdated
Show resolved
Hide resolved
src/main/groovy/org/jenkinsci/gradle/plugins/jpi/TestDependenciesTask.groovy
Outdated
Show resolved
Hide resolved
src/main/groovy/org/jenkinsci/gradle/plugins/jpi/JpiPlugin.groovy
Outdated
Show resolved
Hide resolved
Thanks for the feedback @wolfs. I think this is now in a quite good state. The main thing that remains to be discussed is what to do in terms of compatibility. From my point of view, there are two items:
I think (1) will be quite hard to do. But if, I think the easiest would be to keep two separate implementations around of all classes that were modified in this PR. I think I would prefer one of two options:
Once a decission is made, we should describe it in the README (as suggested by Stefan #132 (comment)). @sghill what is your take on this? You can also ping Stefan and me on the Gradle Slack if you want to discuss directly. |
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 all the updates, and the effort around asserting that the data in the new module files is correct.
I added a few questions, and would prefer to at least explore keeping backwards compatibility with the configurations and Gradle versions.
src/main/groovy/org/jenkinsci/gradle/plugins/jpi/DependencyAnalysis.groovy
Outdated
Show resolved
Hide resolved
@sghill Thank you for bringing up your concerns about backwards compatibility. I wonder though what is blocking folks from upgrading their plugin builds to Gradle 6. I would say that isn't a problem given that the How about we issue PRs to all the plugins under the If the plugin would support both Gradle 6 and earlier versions, then my fear is that features need to be implemented in two different ways, two different sets of tests need to be maintained, etc. For maintainability I think it would be better to keep a |
This is a good point @wolfs. @jjohannes said in #132 (comment):
Yes, I'm OK with making Gradle 6.0 the minimum version for these reasons:
That'd be really appreciated, but to be clear I don't think it's a requirement for merging this excellent change.
Not a big sticking point since we're making the minimum version Gradle 6.0, but I do think Gradle Test Kit changes this equation, making backwards-compatibility cheaper to implement in general. One of the nice things I saw about this approach of (1) create a build file, (2) execute a task, and (3) assert on the outcome meant that different implementations for different versions of Gradle just needed to make the same test pass.
I'm thinking something similar to this, though I'll probably stay on |
This upgrade will require Gradle 6+
Make sure that only pure libraries are packaged and are considered for generating the license XML.
These have no meaning for published metadata. The 'provided' scope is used by Maven for building a local project described by a POM.
This was hiding a real issues with not resolving the Jekins war correctly.
Instead use the lazy configurations mechanisms configurations provide themselves.
Thanks for the additional feedback @sghill. I think the plan you outlined here - #132 (comment) - sounds good. |
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.
LGTM!
This was published in a prerelease today: v0.39.0-rc.1 |
Thank you for moving so quickly with this @sghill! |
@jjohannes I tried it out in the Moreover, there are quite some warnings that spotbugs doesn't find some classes. Can you take a look? |
The changes in #135 have been pre-released as v0.39.0-rc.2 today. |
This is a larger update of the plugin discussed with @wolfs. Following the discussions on #128 and beyond.
It uses the new variant-aware dependency management capabilities of Gradle and Gradle Module Metadata to better model the different variants of component (
jar
vs.jpi
) in the Jenkins ecosystem.This is WiP for further discussion for now.