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

Gradle project is created with deprecated features (Project.getConvention) #1423

Closed
wants to merge 30 commits into from

Conversation

prithvitewatia
Copy link
Contributor

This PR closes #1420. In this PR gradle build project generation convention is updated to use Java configuration instead of sourceCompatibility property as they are deprecated and scheduled for removal in gradle 9.0.
Upgrading source compatibility.

As so many test resources files had been updated to validate project generation according, I have tried to make small logical units of commits that may be helpful in reviewing this PR:

  1. Commit 1. In this commit, I have simply updated .gen files to use new java configuration that is common for both groovy and kotlin.
  2. Commit 2 In this commit, I have added a method to write java configuration instead of source compatibility.
  3. Commit 3 In this commit, I have added a method that returns a string representation for java configuration to be used in tests.
  4. Commit 4 In this commit, I have updated the source compatibility test for Kotlin Dsl to check for java configuration.
  5. Commit 5 In this commit, I have updated the source compatibility test for groovy dsl to check for java configuration and parameterised this test.
  6. Commit 6 In this method I have updated method to check for java source compatibility
  7. Commit 7 In this
    commit, I have removed method that used to write source compatibility. Also created a class that will provide java version constant.
  8. Commit 8 Updated tests to check for Java configuration
  9. Commit 9 Added java configuration in gradle build settings.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jun 7, 2023
Copy link
Contributor

@snicoll snicoll left a comment

Choose a reason for hiding this comment

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

Thanks very much for the PR, I've added a comment for your consideration.

@@ -6,7 +6,9 @@ plugins {

group = 'com.example'
version = '0.0.1-SNAPSHOT'
sourceCompatibility = '1.8'
java {
Copy link
Contributor

Choose a reason for hiding this comment

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

That's an odd place to put this. Now that it isn't an attribute anymore, it should go after the dependencies IMO.

Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW, I wouldn't move it. How the information is expressed doesn't change the importance of that information so I'd keep it where it is, although ideally preceded by a blank line.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, I guess I am biased with how things work with Maven. It's all good keeping it there and yes with a blank line.

@snicoll snicoll added the status: waiting-for-feedback We need additional information before we can continue label Jun 8, 2023
@snicoll
Copy link
Contributor

snicoll commented Jun 8, 2023

We should consider also #211 as part of this as we could simply remove the code for Kotlin-based projects.

@@ -118,7 +118,9 @@ void buildDotGradleIsContributedWhenGeneratingGradleProject() {
"",
"group = 'com.example'",
"version = '0.0.1-SNAPSHOT'",
"sourceCompatibility = '11'",
"java {",
" sourceCompatibility = JavaVersion.VERSION_11",
Copy link
Contributor

@wilkinsona wilkinsona Jun 8, 2023

Choose a reason for hiding this comment

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

I'd prefer that we continue to use a String for the version as it's more concise and, IMO, it reads better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@snicoll @wilkinsona, I read in https://docs.gradle.org/8.2-rc-1/dsl/org.gradle.api.plugins.JavaPluginExtension.html#org.gradle.api.plugins.JavaPluginExtension:sourceCompatibility
that source compatibility is going to be like java version constant. That is why I added it. So should we continue with String for version or add Java version constant ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Gradle will perform the Object -> JavaVersion conversion as it's https://docs.gradle.org/8.2-rc-1/javadoc/org/gradle/api/plugins/JavaPluginExtension.html#setSourceCompatibility-java.lang.Object- that will be called.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. I understand now.

@prithvitewatia
Copy link
Contributor Author

@snicoll @wilkinsona Thanks for the review. I have one question. Currently in Gradle Build Settings I have added source compatibility as this is the only field we used to generate. However many other options are possible for java configuration as mentioned here. Should I also add them or continue with source compatibility ?

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Jun 8, 2023
* Class to map common configuration for Java based projects.
*
*/
public static class JavaConfiguration {
Copy link
Contributor

@wilkinsona wilkinsona Jun 8, 2023

Choose a reason for hiding this comment

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

I wonder if we need this new public API? The change could be implemented by only changing the writers. We could wait to introduce a new API until a need arises to configure more of the options on JavaPluginExtension.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I agree that it's a bit early to introduce a new API for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the clarification. I will update the PR accordingly

@prithvitewatia
Copy link
Contributor Author

@snicoll @wilkinsona Requested changes have been made. I have also made changes for #211. Please review it when possible.

@snicoll snicoll changed the title Gh 1420. Updated Project generation convention to use java configuration Gradle project is created with deprecated features (Project.getConvention) Jun 12, 2023
@snicoll snicoll added type: bug and removed status: feedback-provided Feedback has been provided status: waiting-for-triage An issue we've not yet triaged labels Jun 12, 2023
@snicoll snicoll self-assigned this Jun 12, 2023
@snicoll snicoll added this to the 0.20.0 milestone Jun 12, 2023
@snicoll
Copy link
Contributor

snicoll commented Jun 12, 2023

@prithvitewatia I don't think that works. There is a difference between the Kotlin DSL and Kotlin as a language chosen for the application. We no longer generate a sourceCompatibility when the Kotlin DSL is chosen, whereas it should have been when Kotlin is chosen as a language.

Anyway, I don't like the idea of mixing two changes in the same PR. If you want us to consider this PR, please only focus on changing how the source compatibility is written, and ignoring #211 for now. The changes in the Groovy writer is a bit too invasive as well so hopefully this should simplify it.

Let me know if you'd like to review the PR or if you want me to take over.

@snicoll snicoll added the status: waiting-for-feedback We need additional information before we can continue label Jun 12, 2023
@prithvitewatia
Copy link
Contributor Author

@snicoll I might have intermixed two things. For now, I am planning to do changes only for #1423 in a new branch to better track changes.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Jun 12, 2023
@snicoll
Copy link
Contributor

snicoll commented Jun 13, 2023

Superseded by #1427

@snicoll snicoll closed this Jun 13, 2023
@snicoll snicoll removed this from the 0.20.0 milestone Jun 13, 2023
@prithvitewatia prithvitewatia deleted the gh-1420 branch June 14, 2023 12:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Gradle project is created with deprecated features (Project.getConvention)
4 participants