Skip to content

Conversation

@timhamoni
Copy link

I've added the ability to specify a platform constraint to the detachedConfiguration used to resolve the dependencies. This makes it easier for projects that have specific versions enforced to resolve artifacts.

Copy link
Member

@jjohannes jjohannes left a comment

Choose a reason for hiding this comment

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

Thank you for the suggestion and contribution.

The challenge in this area is to get consistent versions "everywhere" in a multi-project. We/I struggled with this topic. The current solution I use in the projects that use this plugin is to set up a consistent resolution classpath which basically is a "Resolvable Configuration" that has dependencies to everything in a multi-project. (There is some support for such setups now in our jvm-dependency-conflict-resolution plugin).

It feels like this could be simplified if I already have a platform in my project that has all the versions already.

That's why I think this is a good addition and it may be sufficient to make certain setups a bit simpler.

I have only a few small remarks about code consistency and readability and one thing to consider in the "DSL design" aspect of this.

Comment on lines +102 to +103
if (!ModuleDependency.class.isAssignableFrom(
platformDependencyProvider.get().getClass())) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (!ModuleDependency.class.isAssignableFrom(
platformDependencyProvider.get().getClass())) {
if (!(platformDependencyProvider.get() instanceof ModuleDependency)) {

💄 For consistency: do it similar as other instanceof checks in this codebase.

extraJavaModuleInfo {
failOnAutomaticModules.set(true)
platformDependency.set(project.provider { project.dependencies.platform(project.dependencies.create(springBom)) })
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
platformDependency.set(project.provider { project.dependencies.platform(project.dependencies.create(springBom)) })
platformDependency.set(provider { dependencies.platform(dependencies.create(springBom)) })

💄 consistent with doc change


```kotlin
extraJavaModuleInfo {
platformDependency = project.provider { project.dependencies.platform(project.dependencies.create("org.example:bom:1.0.0")) }
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
platformDependency = project.provider { project.dependencies.platform(project.dependencies.create("org.example:bom:1.0.0")) }
platformDependency = provider { dependencies.platform(dependencies.create("org.example:bom:1.0.0")) }

💄 We are always in the project context here. Lets do this as short as possible.

Would be great if there would be ca notation without provider{ }, but I think there is no way to do that.


Alternatively, we could think about providing one (or two) String-based method as for the dependency:

   platformDependency = "org.example:bom:1.0.0"
   platformDependencyStrict = true

Then, you won't need the validation and instead could always create a platform dependency from the coordinates. I like this a bit better, as it is easier to read for users that skim the build scripts and do not know all the details. It looks more like a DSL then. You would need to check if the string is coordinates or project name (starts with :) to decide if you need to project(...) when creating the dependency. This would be consistent with what we do here.
But I think I am also fine with the current solution. What do you think?

def "resolve against a platform project if specified"() {
given:
buildFile << """
val springBom = "org.springframework:spring-framework-bom:6.2.9"
Copy link
Member

Choose a reason for hiding this comment

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

The test fails on Gradle 6.x because it is hard (probably impossible) to run that with Java versions >11. Maybe just choose an older version of Sping boot here that does not require Java 17 as minimum as it is just a test example.

We may drop support for 6.x soon, but that's a different story. :)


In addition to creating a common configuration for resolution, a platform project can also be enabled to enforce
versions.

Copy link
Member

Choose a reason for hiding this comment

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

Okay, I have one more question for clarification and to understand what setups we can recommend to users. If you set versionsProvidingConfiguration to a configuration that resolves all dependencies that you compute from your platform (see discussion in #207), then you don't need the addition of this PR, right?

So this is an alternative to versionsProvidingConfiguration, correct? Which then indeed makes the setup more concise as you do not have to deal with all the ceremony around versionsProvidingConfiguration (see my comment here).

Copy link
Member

Choose a reason for hiding this comment

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

To partly answer my own question:

  • The versionsProvidingConfiguration approach has the advantage (may be considered a disadvantage) that it also provides the versions of all (well, almost all) transitive dependencies. In your own platform project, you may only define versions of you direct dependencies. Then this approach still works.
  • The platformDependency approach (introduced in this PR) requires you to define all versions in the platform, but has the advantage of a simpler overall setup as I wrote above. Maybe you want to define all versions anyway to have more control and overview over all transitives. Or you mainly rely on an existing platform, like spring-framework-bom, that defines all the versions for you already.

I think then, for the moment, both are valid approaches and adding this PR, while also keeping versionsProvidingConfiguration, is a good addition.

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.

2 participants