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.

Sign up for GitHub

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

Jump to bottom

compile grails-shell, grails-gradle-model and grails-bootstrap w/ Gradle Groovy version #13653

Merged
merged 29 commits into from
Sep 22, 2024
Merged

compile grails-shell, grails-gradle-model and grails-bootstrap w/ Gradle Groovy version #13653

merged 29 commits into from
Sep 22, 2024

Conversation

jamesfredley
Copy link
Contributor

@jamesfredley jamesfredley commented Sep 18, 2024 •
edited
Loading

compile grails-shell, grails-gradle-model and grails-bootstrap with the Groovy version provided by Gradle to ensure build compatibility with Gradle, currently Groovy 3.0.x see: https://docs.gradle.org/current/userguide/compatibility.html#groovy

This will prevent running into Groovy 4 vs 3 compatibility issues like: bertramdev/asset-pipeline#350, when Gradle executes the build using it's embedded Groovy Version, currently 3.0.22.

Matches changes on grails/grails-gradle-plugin#334

@jamesfredley jamesfredley mentioned this pull request Sep 18, 2024
Merged
matrei
matrei reviewed Sep 19, 2024
View reviewed changes
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.

grails-bootstrap was not changed due to Groovy XML package changes in 4.

Aren't the XML classes available in both groovy.util and groovy.xml in Groovy 3.
Are there other package changes that are incompatible?

@jamesfredley
Copy link
Contributor Author

PR updated to include grails-bootstap. I wasn't aware of the dual packaging for XML and must have been missing a dependency during the first attempt.

matrei
matrei reviewed Sep 19, 2024
View reviewed changes
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.

Nice! However, there is another dependency on org.apache.groovy:groovy-xml. Is that supposed to be left in?

grails-shell/build.gradle Outdated
@@ -20,7 +27,7 @@ dependencies {
api "org.fusesource.jansi:jansi:$jansiVersion"
api "jline:jline:$jlineVersion"
api "org.gradle:gradle-tooling-api:$gradleToolingApiVersion"
compileOnly "org.apache.groovy:groovy-templates:$groovyVersion"

api "org.apache.groovy:groovy-xml:$groovyVersion"
Copy link
Contributor

Choose a reason for hiding this comment

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

How about this one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I left this one as org.apache.groovy and 4.0.x since it was not for compilation and is needed by at least one project down the line that is running it with Groovy 4 (not grails-gradle-plugin)

We did the same on https://github.com/bertramdev/asset-pipeline/blob/5.0.x/asset-pipeline-core/build.gradle#L56-L74

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, but the one in asset-pipeline is testImplementation. This one is api. Doesn't api put it on the compileClasspath?

@matrei
Copy link
Contributor

matrei commented Sep 19, 2024

Don't we also have to exclude these projects from the Groovy 3 -> Groovy 4 substitution in the root build.gradle or they will just be replaced with Groovy 4 again?

@jamesfredley jamesfredley requested a review from matrei September 20, 2024 23:03
@matrei
Copy link
Contributor

matrei commented Sep 21, 2024

Related grails/grails-gradle-plugin#337

matrei
matrei reviewed Sep 21, 2024
View reviewed changes
build.gradle Outdated Show resolved Hide resolved
grails-bootstrap/build.gradle Outdated Show resolved Hide resolved
@jamesfredley jamesfredley mentioned this pull request Sep 21, 2024
Merged
@jamesfredley
Copy link
Contributor Author

Dependency Insight now looks good for the 3 subprojects.

.gradlew grails-gradle-model:dI --dependency groovy --configuration compileClasspath
.gradlew grails-shell:dI --dependency groovy --configuration compileClasspath
.gradlew grails-bootstrap:dI --dependency groovy --configuration compileClasspath

groovydocs required several changes, including moving where and how we set JavaLanguageVersion.of(17), there are still some errors when JavaParser runs related to pattern and instanceOf. These existed prior to these changes and do not stop the build.

I've tested grails-core -> grails-gradle-plugin -> grails-profile base to ensure that dependencies are still passed as expected. grails-profile base is a good test case because it receives almost all of its dependencies from grails-gradle-plugin, which inherits some from grails-boostrap and grails-shell in grails-core.

@jamesfredley
Copy link
Contributor Author

grails-core does not use grails-gradle-plugin during build so grails/grails-gradle-plugin#337 does not impact these changes.

@matrei
fix: get groovydoc task working again
a926e13 
Reverts some changes that updated to Java 17 syntax `instanceof`. At the moment this breaks groovydoc generation with the message:
Use of patterns with instanceof is not supported. Pay attention that this feature is supported starting from 'JAVA_14' language level. If you need that feature the language level must be configured in the configuration before parsing the source files.

As these changes are purely cosmetic I'm find it better to revert them.
@jamesfredley jamesfredley linked an issue Sep 22, 2024 that may be closed by this pull request
Grails 7: groovydoc/JavaParser - ignored errors parsing Java source file #13667
Closed
@jamesfredley jamesfredley changed the title compile grails-shell and grails-gradle-model w/ Gradle Groovy version compile grails-shell. grails-gradle-model and grails-bootsrap w/ Gradle Groovy version Sep 22, 2024
@jamesfredley jamesfredley changed the title compile grails-shell. grails-gradle-model and grails-bootsrap w/ Gradle Groovy version compile grails-shell, grails-gradle-model and grails-bootstrap w/ Gradle Groovy version Sep 22, 2024
@jamesfredley jamesfredley merged commit d535624 into 7.0.x Sep 22, 2024
11 checks passed
@jamesfredley jamesfredley deleted the jamesfredley/fix-use-gradle-groovy-version branch September 22, 2024 20:47
@jamesfredley jamesfredley mentioned this pull request Sep 23, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matrei matrei matrei approved these changes

@puneetbehl puneetbehl Awaiting requested review from puneetbehl

Assignees
No one assigned
Labels
None yet
Projects
Grails 7
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Grails 7: groovydoc/JavaParser - ignored errors parsing Java source file
2 participants
@jamesfredley @matrei