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

7.0.x dependency cleanup #13787

Open
wants to merge 76 commits into
base: 7.0.x
Choose a base branch
from

Conversation

codeconsole
Copy link
Contributor

@codeconsole codeconsole commented Oct 18, 2024

I think this will be a lot easier to maintain. All version numbers have been pulled out and the entire grails-core repo now uses its own bom for dependency version resolution.

I provided a more concise, clean approach to listing dependency versions. The previous way was a bit more tedious and time consuming having to add a map entry for every dependency.

Check out those deletion stats 😉

Still work needs to be done to analyze dependency configurations, but this can be done in a later ticket.

Spring Bom
Groovy Bom

gradle.properties Outdated Show resolved Hide resolved
@matrei
Copy link
Contributor

matrei commented Oct 18, 2024

The build is failing after the last cleanup commit 80534e7.
Could not find jline:jline: and Could not find com.github.javaparser:javaparser-core:

Copy link
Contributor

@matrei matrei left a comment

Choose a reason for hiding this comment

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

Great work Scott!

Can we add the versions as properties in the pom like in https://repo1.maven.org/maven2/org/springframework/boot/spring-boot-dependencies/3.3.4/spring-boot-dependencies-3.3.4.pom
Like it is for the plugins and profiles.
Then it is easy to override the version ext.set('spring.version', '6.1.13')

There is also some overlap between spring-boot-dependencies and grails-bom. Do we want to clean that up?

org.grails:grails-gdoc-engine::$gdocEngineVersion
org.grails:grails-gradle-plugin::$grailsGradlePluginVersion
org.grails:grails-testing-support,grails-gorm-testing-support,grails-web-testing-support::$testingSupportVersion
org.grails:grails:gsp,web-gsp:$gspVersion
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm missing some gsp modules here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what would you like added?

Copy link
Contributor

Choose a reason for hiding this comment

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

In grails-mail I wanted to use org.grails:grails-web-taglib but it is missing from the bom.
I guess all modules from grails-gsp should be added.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

web can clean it up. we just need to identify the overlap and make sure the versions don't need to be overridden.
if they are the same version, we should remove.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

adding the versioning is going to be a bit of work. I'd prefer to tackle that later. I made some attempts at it, but haven't had success yet. I haven't come up with or found a simple way of doing it yet. The previous version that worked was just building a maven bom which couldn't be used by the other projects. This is actually creating a bom type project.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@matrei I just didn't manually. Take a look, you have versions now

Copy link
Contributor

Choose a reason for hiding this comment

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

👍
Right now spock-core and spock-spring get included twice the pom.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

grails-bom/build.gradle Outdated Show resolved Hide resolved
grails-bom/build.gradle Outdated Show resolved Hide resolved
grails-bom/build.gradle Outdated Show resolved Hide resolved
@codeconsole
Copy link
Contributor Author

codeconsole commented Oct 18, 2024

The build is failing after the last cleanup commit 80534e7

I guess it doesn't like to resolve bom versions for documentation dependencies

        documentation("org.fusesource.jansi:jansi:$jansiVersion")
        documentation("jline:jline:$jlineVersion")
        documentation ("com.github.javaparser:javaparser-core:$javaParserCoreVersion")

I added reverted the versions back.

@matrei
Copy link
Contributor

matrei commented Oct 18, 2024

I guess it doesn't like to resolve bom versions for documentation dependencies

Wouldn't documentation platform(project(':grails-bom')) add it?

Copy link
Contributor

@jamesfredley jamesfredley left a comment

Choose a reason for hiding this comment

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

The few comments, plus I'd like to add these:

org.gebish:geb-spock:7.0
com.bertramlabs.plugins:asset-pipeline-grails:5.0.1
org.grails.plugins:database-migration:6.0.0-SNAPSHOT

@codeconsole
Copy link
Contributor Author

codeconsole commented Oct 21, 2024

The few comments, plus I'd like to add these:
org.gebish:geb-spock:7.0 com.bertramlabs.plugins:asset-pipeline-grails:5.0.1 org.grails.plugins:database-migration:6.0.0-SNAPSHOT

@jamesfredley comments resolved, dependencies added

@@ -7,12 +7,16 @@ dependencies {
api "org.apache.ant:ant:${project['ant.version']}"
api "org.apache.ant:ant-junit:${project['ant.version']}"
api "org.asciidoctor:asciidoctorj:${project['asciidoctorj.version']}"
api "com.bertramlabs.plugins:asset-pipeline:${project['asset-pipeline.version']}"
Copy link
Contributor

@matrei matrei Oct 21, 2024

Choose a reason for hiding this comment

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

com.bertramlabs.plugins:asset-pipeline is not right. Did you mean asset-pipeline-core?
Also, these versions has historically been out of sync. For example, there is no asset-pipeline-grails:4.4.0.

Copy link
Contributor Author

@codeconsole codeconsole Oct 21, 2024

Choose a reason for hiding this comment

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

ok, poor assumption on my part. removed.

@jamesfredley
Copy link
Contributor

@codeconsole
Copy link
Contributor Author

codeconsole commented Oct 21, 2024

@matrei if I new I was going to make so many changes, I would have separated the task names a long time ago when you first asked. lol.

🤣

@matrei
Copy link
Contributor

matrei commented Oct 21, 2024

These are the version property names for the jakarta dependencies in spring-boot-dependencies that we duplicate. Should we try and keep the property names in sync or remove these dependences from grails-bom if we don't need to set different versions?
Right now our property names are jakarta-*-api.version

<jakarta-annotation.version>2.1.1</jakarta-annotation.version>
<jakarta-inject.version>2.0.1</jakarta-inject.version>
<jakarta-persistence.version>3.1.0</jakarta-persistence.version>
<jakarta-servlet.version>6.0.0</jakarta-servlet.version>
<jakarta-xml-bind.version>4.0.2</jakarta-xml-bind.version>

As much as I would like to support patch updates. The right thing is prob to resolve to Spring.

removed...

…ady requires a configuration exclusion. The locations that requires it should be updated to the current hamcrest version already referenced in the spring bom.
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.

4 participants