Use the `implementation` configuration to replace `compile` #40

supl · 2023-05-22T16:49:45Z

This PR revises the Gradle build system.

It uses the implementation configuration to replace compile.
This is because the compile configuration is deprecated and using the compile configuration doesn't include the dependencies into the jar file.

supl · 2023-05-22T16:52:21Z

node/package.json

@@ -1,6 +1,6 @@
 {
  "name": "@scalar-labs/scalar-admin",
-  "version": "2.1.0",
+  "version": "2.1.1",


we update this because now the version of Java and Node are aligned so they have to be updated at the same time.

feeblefakie · 2023-05-24T01:10:17Z

java/build.gradle

@@ -102,7 +102,7 @@ targetCompatibility = 1.8

 group = "com.scalar-labs"
 archivesBaseName = "scalar-admin"
-version = "2.1.0"
+version = "2.1.1"


Is it a bug fix? or an improvement? It's labeled as improvement, so they look inconsistent.

Sorry, it's a mistake on label.
It should be a bugfix.

I feel it's an improvement.

So after we merge this, are we going to release 2.1.1? Actually, I don't think we need to release it right away.

@brfrn169

Thank you for the advise.

I see. Let me revise it as an improvement.

I was thinking it's a bugfix because:

For example, I am using scalar-admin's RequestCoordinator class in another project scalar-admin-k8s (an additional tool for pausing Scalar products in Kubernetes environemtn).

RequestCoordinator depends on, for an instance, the dnsjava package.

However, because the dnsjava dependency is declared with compile here, so it won't be included in the runtime classpath.

In other words, the scalar-admin-k8s using RequestCoordinator needs to declare dnsjava as a runtime dependency by itself.

This is what I add additionally to the build.gradle of scalar-admin-k8s.

// FIXME these declarations are due to the drawbacks of using the `compile` configuration in scalar-admin 2.1.0. // Remove these declarations when it's fixed in scalar-admin after 2.1.0. implementation group: 'javax.json', name: 'javax.json-api', version: '1.1.4' implementation group: 'org.glassfish', name: 'javax.json', version: '1.1.4' implementation group: 'dnsjava', name: 'dnsjava', version: '3.5.2' implementation group: 'io.grpc', name: 'grpc-alts', version: '1.51.0' implementation group: 'io.grpc', name: 'grpc-netty', version: '1.51.0' implementation group: 'io.grpc', name: 'grpc-protobuf', version: '1.51.0' implementation group: 'io.grpc', name: 'grpc-stub', version: '1.51.0' implementation group: 'com.google.api.grpc', name: 'proto-google-common-protos', version: '1.0.0' implementation group: 'com.google.api.grpc', name: 'googleapis-common-protos', version: '0.0.3' implementation group: 'com.google.protobuf', name: 'protobuf-java-util', version: '3.21.12'

(almost copied the whole dependency configuration from scalar-admin)

From scalar-admin-k8s perspective, I feel it's weird to do this because scalar-admin-k8s doesn't directly depends on these packages. The indirected packages should be declared at the library level (scalar-admin) which directly depends on these packages.

I think the issue won't happen in scalardl and scalardb because they only use the gRPC generated classes, and the generated classes depend on grpc-core which is for sure declared in scalardl and scalardb.

Sounds strange. If you declare the dependencies with compileOnly, that would make sense. But if you declare the dependencies with compile, they should be included in the class path.

But, I checked the pom file of scalar-admin-2.1.0, and surprisingly it doesn't have any dependencies:
https://repo1.maven.org/maven2/com/scalar-labs/scalar-admin/2.1.0/scalar-admin-2.1.0.pom

For example, this is the build.gradle file for ScalarDB 3.0.0:
https://github.com/scalar-labs/scalardb/blob/v3.0.0/build.gradle

In this file, the dependencies are declared with compile (and testCompile).

And the pom file for this release is as follows:
https://repo1.maven.org/maven2/com/scalar-labs/scalardb/3.0.0/scalardb-3.0.0.pom

The dependencies are included in the dependencies part in pom.xml.

I think that's a normal situation.

@brfrn169

Thanks for your hint!

I found the magic occurs between the legacy maven plugin and the current maven-publish plugin. They are in charge of creating POM files.

This is the POM file of scalar-admin 1.1.0
https://repo1.maven.org/maven2/com/scalar-labs/scalar-admin/1.1.0/scalar-admin-1.1.0.pom
(the compile dependencies are there)

and, before 1.2.0, I made this commit to replace the legacy maven plugin with maven-publish
72replacingc8237f7d181b702d99acd63dd3134955110312

it caused the POM file of scalar-admin 1.2.0
https://repo1.maven.org/maven2/com/scalar-labs/scalar-admin/1.2.0/scalar-admin-1.2.0.pom (the dependencies are gone)

It seems that the legacy maven plugin puts the compile dependencies into the POM but the maven-publish plugin doesn't.

And I found a source code snippet of the maven-publish plugin.

https://github.com/gradle/gradle/blob/master/subprojects/maven/src/main/java/org/gradle/api/publish/maven/plugins/MavenPublishPlugin.java#L224

The maven-publish seems to treat the compile dependency as the api dependency, which will not be included in the runtime classpath. Then it explains...

By the way, I guess the reason I switched to maven-plugin is in order to publish it to local maven repo and test it with another project.

It looks like an api dependency in build.gradle is interpreted as a compile scope dependency in pom.xml.

build.gradle:
https://github.com/scalar-labs/scalardb/blob/v3.9.0/rpc/build.gradle

pom.xml:
https://repo1.maven.org/maven2/com/scalar-labs/scalardb-rpc/3.9.0/scalardb-rpc-3.9.0.pom

So the maven-publish plugin just ignores the compile dependencies in build.gradle? That's weird...

Is this related?
https://discuss.gradle.org/t/maven-publish-plugin-does-not-include-dependencies-in-generated-pom-xml-file/10113

brfrn169 · 2023-05-25T00:10:01Z

java/build.gradle

-    compile group: 'io.grpc', name: 'grpc-stub', version: "${grpcVersion}"
-    compile group: 'com.google.protobuf', name: 'protobuf-java-util', version: "${protobufVersion}"
-    compile group: 'dnsjava', name: 'dnsjava', version: '2.1.9'
+    implementation group: 'org.slf4j', name: 'slf4j-api', version: "${slf4jVersion}"


How about using the short format?

Suggested change

implementation group: 'org.slf4j', name: 'slf4j-api', version: "${slf4jVersion}"

implementation "org.slf4j:slf4j-api:${slf4jVersion}"

Thanks for the advise!

I revise it in cd2c4b1

Please take a look 🙇

brfrn169 · 2023-05-25T00:13:43Z

java/build.gradle

@@ -102,7 +102,7 @@ targetCompatibility = 1.8

 group = "com.scalar-labs"
 archivesBaseName = "scalar-admin"
-version = "2.1.0"
+version = "2.1.1"


I feel it's an improvement.

So after we merge this, are we going to release 2.1.1? Actually, I don't think we need to release it right away.

feeblefakie · 2023-06-07T02:31:33Z

@supl What should we do for this PR?

supl · 2023-06-07T05:34:14Z

@supl What should we do for this PR?

@feeblefakie
@brfrn169

Sorry for taking so long.

Regarding the feedback from Toshi
#40 (comment)

I was trying to find why the maven-publish plugin ignores the dependencies with the compile configurations in these days but I still couldn't find it.

However, I think it's a correct change to replace the compile configuration by the implementation configuration because:

maven-publish plugin works fine with it
the compile configuration is deprecated after Gradle 7.0 after all

So, I think I will stop digging the maven-publish plugin and just use the implementation to fix the drawback (the dependencies are not included in the POM file)

brfrn169

Left one comment. Other than that, LGTM! Thank you!

java/build.gradle

Co-authored-by: Toshihiro Suzuki <brfrn169@gmail.com>

supl · 2023-06-07T06:17:31Z

I also change the version of the node package to 2.1.1 to align the Java's version.
c483aa3

feeblefakie · 2023-06-09T00:59:07Z

@supl What should we do about this? Is it OK with the bugfix label?

supl · 2023-06-09T15:02:22Z

@supl What should we do about this? Is it OK with the bugfix label?

@feeblefakie

I think we can merge it and it should be a bugfix.

supl added the improvement label May 22, 2023

supl requested a review from feeblefakie May 22, 2023 16:49

supl self-assigned this May 22, 2023

Use the implementation configuration to replace compile

d1a4e6e

supl force-pushed the use-implementation branch from 263857b to d1a4e6e Compare May 22, 2023 16:51

supl commented May 22, 2023

View reviewed changes

supl mentioned this pull request May 22, 2023

Implement Pauser scalar-labs/scalar-admin-for-kubernetes#1

Closed

feeblefakie requested a review from brfrn169 May 24, 2023 01:08

feeblefakie reviewed May 24, 2023

View reviewed changes

supl added bug Something isn't working and removed improvement labels May 24, 2023

supl requested a review from feeblefakie May 24, 2023 08:14

brfrn169 reviewed May 25, 2023

View reviewed changes

supl requested a review from brfrn169 May 25, 2023 02:28

supl added 2 commits May 25, 2023 11:29

Make it as an improvement version

7ec7d84

Use short declarations

cd2c4b1

supl added improvement and removed bug Something isn't working labels May 25, 2023

Change the version of Node too

a00431a

brfrn169 approved these changes Jun 7, 2023

View reviewed changes

java/build.gradle Outdated Show resolved Hide resolved

Update java/build.gradle

ba2c132

Co-authored-by: Toshihiro Suzuki <brfrn169@gmail.com>

supl added bugfix and removed improvement labels Jun 7, 2023

2.1.1 for the node package

c483aa3

feeblefakie approved these changes Jun 9, 2023

View reviewed changes

feeblefakie merged commit d2d1ee3 into main Jun 9, 2023
1 check passed

feeblefakie deleted the use-implementation branch June 9, 2023 15:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use the `implementation` configuration to replace `compile` #40

Use the `implementation` configuration to replace `compile` #40

supl commented May 22, 2023

supl May 22, 2023

feeblefakie May 24, 2023

supl May 24, 2023

brfrn169 May 25, 2023

supl May 25, 2023

brfrn169 May 25, 2023

brfrn169 May 25, 2023 •

edited

supl May 25, 2023 •

edited

supl May 25, 2023

brfrn169 May 25, 2023 •

edited

brfrn169 May 25, 2023

brfrn169 May 25, 2023

supl May 25, 2023 •

edited

brfrn169 May 25, 2023

feeblefakie commented Jun 7, 2023

supl commented Jun 7, 2023

brfrn169 left a comment

supl commented Jun 7, 2023

feeblefakie commented Jun 9, 2023

supl commented Jun 9, 2023

	implementation group: 'org.slf4j', name: 'slf4j-api', version: "${slf4jVersion}"
	implementation "org.slf4j:slf4j-api:${slf4jVersion}"

Use the implementation configuration to replace compile #40

Use the implementation configuration to replace compile #40

Conversation

supl commented May 22, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brfrn169 May 25, 2023 • edited

Choose a reason for hiding this comment

supl May 25, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brfrn169 May 25, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

supl May 25, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feeblefakie commented Jun 7, 2023

supl commented Jun 7, 2023

brfrn169 left a comment

Choose a reason for hiding this comment

supl commented Jun 7, 2023

feeblefakie commented Jun 9, 2023

supl commented Jun 9, 2023

Use the `implementation` configuration to replace `compile` #40

Use the `implementation` configuration to replace `compile` #40

brfrn169 May 25, 2023 •

edited

supl May 25, 2023 •

edited

brfrn169 May 25, 2023 •

edited

supl May 25, 2023 •

edited