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

Replace deprecated Gradle/ScalarDB features #74

Merged
merged 5 commits into from
Nov 20, 2024
Merged

Conversation

KodaiD
Copy link
Contributor

@KodaiD KodaiD commented Nov 11, 2024

Description

This PR replaces deprecated Gradle/ScalarDB features to suppress warning messages.

The sample applications of ScalarDB use some deprecated Gradle features and put API of ScalarDB. Due to these reasons, multiple warning messages are displayed when the sample application is executed, which is annoying to users.

So this PR replaces all deprecated Gradle features and replaces put API of ScalarDB with update/insert.

Related issues and/or PRs

N/A

Changes made

  • Replace deprecated Gradle features.
  • Replace put API of ScalarDB with update/insert.

Checklist

  • I have commented my code, particularly in hard-to-understand areas.
  • I have updated the documentation to reflect the changes.
  • Any remaining open issues linked to this PR are documented and up-to-date (Jira, GitHub, etc.).
  • Tests (unit, integration, etc.) have been added for the changes.
  • My changes generate no new warnings.
  • Any dependent changes in other PRs have been merged and published.

Additional notes (optional)

N/A

@KodaiD KodaiD self-assigned this Nov 11, 2024
Comment on lines 12 to 14
application {
mainClassName = 'sample.client.Client'
mainClass = 'sample.client.Client'
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This fix suppress the following warning message:

The org.gradle.api.plugins.ApplicationPluginConvention type has been deprecated. This is scheduled to be removed in Gradle 9.0. Consult the upgrading guide for further information: https://docs.gradle.org/8.5/userguide/upgrading_version_8.html#application_convention_deprecation

Comment on lines -16 to +18
archivesBaseName = "sample-order-service"
base {
archivesName = "sample-order-service"
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This fix suppress the following warning message:

The org.gradle.api.plugins.BasePluginConvention type has been deprecated. This is scheduled to be removed in Gradle 9.0. Consult the upgrading guide for further information: https://docs.gradle.org/8.5/userguide/upgrading_version_8.html#base_convention_deprecation

Comment on lines -18 to +24
sourceCompatibility = 1.8
targetCompatibility = 1.8
java {
toolchain {
languageVersion = JavaLanguageVersion.of(8)
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This fix suppress the following warning message:

The org.gradle.api.plugins.JavaPluginConvention type has been deprecated. This is scheduled to be removed in Gradle 9.0. Consult the upgrading guide for further information: https://docs.gradle.org/8.5/userguide/upgrading_version_8.html#java_convention_deprecation

id 'com.google.protobuf' version '0.9.1'
id 'com.google.protobuf' version '0.9.4'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

To suppress the following warning message, com.google.protobuf must be upgraded.

The Provider.forUseAtConfigurationTime method has been deprecated. This is scheduled to be removed in Gradle 9.0. Simply remove the call. Consult the upgrading guide for further information: https://docs.gradle.org/8.5/userguide/upgrading_version_7.html#for_use_at_configuration_time_deprecation

@@ -20,16 +20,40 @@ protobuf {
generateProtoTasks {
all()*.plugins { grpc {} }
}
generatedFilesBaseDir = "$projectDir/src"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

generatedFilesBaseDir is deprecated in com.google.protobuf v0.9.2- (ref. google/protobuf-gradle-plugin#636).

Comment on lines +29 to +36
// Task copyGeneratedProtoToSrc copies the generated .java files into src directory
task copyGeneratedProtoToSrc(type: Copy) {
description 'Copies generated Protocol Buffer classes to src/main/java/sample/rpc'
dependsOn generateProto
from "$buildDir/generated/source/proto/main/java/sample/rpc"
into "$projectDir/src/main/java/sample/rpc"
duplicatesStrategy = DuplicatesStrategy.INCLUDE
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since generatedFilesBaseDir is deprecated, we need to implement a similar functionality using copy.

Comment on lines 38 to 42
// Task deleteBuildMainJava deletes the generated .java files in build directory
task deleteBuildMainJava(type: Delete) {
dependsOn copyGeneratedProtoToSrc
delete fileTree(dir: "$buildDir/generated/source/proto/main/java/sample/rpc")
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Before running the compileJava task, the files under src/main/java must be deleted, otherwise we will get duplicate class errors like:

xxx/scalardb-samples/microservice-transaction-sample/rpc/build/generated/source/proto/main/java/sample/rpc/CommitRequest.java:9: エラー: クラスsample.rpc.CommitRequestが重複しています
public final class CommitRequest extends

Copy link
Contributor

Choose a reason for hiding this comment

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

This task seems to delete only the generated protobuf files in the build directory, but the current task name might look to delete more. How about adding GeneratedProtobuf or something to the task name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@komamitsu Thanks for reviewing! I applied your suggestion in 8427a0d. PTAL!

Comment on lines -28 to -30
java {
sourceCompatibility = JavaVersion.VERSION_1_8
targetCompatibility = JavaVersion.VERSION_1_8
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because there is kotlin { jvmToolchain(8) }, this setting seems to be unnecessary.

@KodaiD
Copy link
Contributor Author

KodaiD commented Nov 11, 2024

I've confirmed that no warnings appear when executing ./gradlew build --warning-mode all.

scalardb-samples on  fix-to-remove-warnings [!?] took 2s
❯ ./check_build.sh
Processing directory: microservice-transaction-sample
./gradlew build --warning-mode all

BUILD SUCCESSFUL in 828ms
29 actionable tasks: 2 executed, 27 up-to-date
Processing directory: multi-storage-transaction-sample
./gradlew build --warning-mode all

BUILD SUCCESSFUL in 2s
5 actionable tasks: 5 executed
Processing directory: scalardb-kotlin-sample
./gradlew build --warning-mode all

BUILD SUCCESSFUL in 2s
5 actionable tasks: 5 executed
Processing directory: scalardb-sample
./gradlew build --warning-mode all

BUILD SUCCESSFUL in 2s
5 actionable tasks: 5 executed
Processing directory: scalardb-sql-jdbc-sample
./gradlew build --warning-mode all

BUILD SUCCESSFUL in 3s
5 actionable tasks: 5 executed
Processing directory: spring-data-microservice-transaction-sample
./gradlew build --warning-mode all

BUILD SUCCESSFUL in 616ms
29 actionable tasks: 2 executed, 27 up-to-date
Processing directory: spring-data-multi-storage-transaction-sample
./gradlew build --warning-mode all

BUILD SUCCESSFUL in 3s
6 actionable tasks: 6 executed
Processing directory: spring-data-sample
./gradlew build --warning-mode all

BUILD SUCCESSFUL in 3s
6 actionable tasks: 6 executed

Copy link
Contributor

@feeblefakie feeblefakie left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you!

Copy link
Collaborator

@brfrn169 brfrn169 left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you!

Copy link
Contributor

@kota2and3kan kota2and3kan 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 updates!
Overall looks good to me, but I left minor comments.
Please take a look when you have time! 🙇‍♂️

Copy link
Contributor

@Torch3333 Torch3333 left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

Copy link
Contributor

@komamitsu komamitsu left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

Copy link
Contributor

@kota2and3kan kota2and3kan left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you!

@KodaiD KodaiD merged commit bdedc82 into main Nov 20, 2024
32 checks passed
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.

6 participants