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

[#5647] improvement(build): update the jar name to add gravitino prefix #5654

Merged
merged 2 commits into from
Nov 28, 2024

Conversation

orenccl
Copy link
Collaborator

@orenccl orenccl commented Nov 24, 2024

What changes were proposed in this pull request?

As title

Why are the changes needed?

Fix: #5647

Does this PR introduce any user-facing change?

Makes users easy to know it is from Gravitino

How was this patch tested?

Execute Gradle command ./gradlew publishToMavenLocal -x test then check in Maven local path.

@orenccl
Copy link
Collaborator Author

orenccl commented Nov 25, 2024

I also encountered this network error on other pull requests. Is there a way to improve this?

> Task :clients:client-python:downloadHadoopPack FAILED
* What went wrong:
Execution failed for task ':clients:client-python:downloadHadoopPack'.
> A failure occurred while executing de.undercouch.gradle.tasks.download.internal.DefaultWorkerExecutorHelper$DefaultWorkAction
   > java.net.SocketException: Network is unreachable

It seems to be a network issue in the CI environment.

@jerryshao
Copy link
Contributor

jerryshao commented Nov 25, 2024

It is occasional, maybe you can configure that gradle plugin to increase the timeout or add retry times (but it depends on if that plugin has such configurations).

api/build.gradle.kts Outdated Show resolved Hide resolved
@orenccl orenccl marked this pull request as draft November 25, 2024 09:13
@orenccl orenccl force-pushed the improvement/jarName branch from a267a77 to 5ad7d5e Compare November 25, 2024 09:13
@orenccl
Copy link
Collaborator Author

orenccl commented Nov 25, 2024

I've discovered that the artifactId is the jarName without the version.
(Reference: Maven Naming Conventions).

Additionally, the artifactId defaults to project.name.
(Reference: Gradle Documentation for MavenPublication).

Therefore, setting the artifactId only once in the root build.gradle.kts should resolve the issue.

image

image

@orenccl orenccl marked this pull request as ready for review November 25, 2024 09:20
@jerryshao
Copy link
Contributor

For some modules like spark, flink, we already added "gravitino-" as the project name, can you please check if your change will add the prefix again, also please align these modules with other modules.

@orenccl
Copy link
Collaborator Author

orenccl commented Nov 25, 2024

After looking into it, I found that the artifactId set in the root project doesn’t apply if the child build.gradle.kts files have their own artifactId settings.

Moreover, it also looks like they need to do some custom work to get their naming conventions right.

For example

val scalaVersion: String = project.properties["scalaVersion"] as? String ?: extra["defaultScalaVersion"].toString()
val sparkVersion: String = libs.versions.spark33.get()
val sparkMajorVersion: String = sparkVersion.substringBeforeLast(".")
val baseName = "${rootProject.name}-spark-connector-runtime-${sparkMajorVersion}_$scalaVersion"

image

build.gradle.kts Outdated
@@ -413,6 +413,8 @@ subprojects {
artifact(javadocJar)
}

artifactId = "${rootProject.name.lowercase(Locale.getDefault())}-${project.name}"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the invocation to Locale.getDefault()?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was following the code style in this script, where toLowerCase(Locale.getDefault()) is actually just toLowerCase().

Copy link
Contributor

Choose a reason for hiding this comment

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

Specifying Local for lowercase () API is a best practice in JVM language, otherwise we will get an expected result.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Specify locale to en_us
specify Locale

@jerryshao
Copy link
Contributor

I tested locally, LGTM, @orenccl do you have anything else needs to be addressed?

@orenccl
Copy link
Collaborator Author

orenccl commented Nov 26, 2024

Hi, @jerryshao
I believe this is good to go, but happy to address any further comments.

@jerryshao
Copy link
Contributor

@tengqm do you want to take another look?

@tengqm
Copy link
Contributor

tengqm commented Nov 26, 2024

@tengqm do you want to take another look?

Good to learn this convention.
LGTM

@tengqm
Copy link
Contributor

tengqm commented Nov 27, 2024

@tengqm do you want to take another look?

Good to learn this convention. LGTM

p.s. I raised the question because I was checking the use case here. It is not about converting a general text message which are supposed to be localized. My understanding was that JAR names are always ASCII printable characters and we don't need (want to) over-engineer this to consider file names with "イ" or "い".

@jerryshao
Copy link
Contributor

@tengqm do you want to take another look?

Good to learn this convention. LGTM

p.s. I raised the question because I was checking the use case here. It is not about converting a general text message which are supposed to be localized. My understanding was that JAR names are always ASCII printable characters and we don't need (want to) over-engineer this to consider file names with "イ" or "い".

I see. Yes, it depends. But some static check tools like error-prone, or IDEA itself may complain about this if the Locale is not set. So typically, we will add the Locale to the methods like this.

@jerryshao
Copy link
Contributor

Hi @tengqm I'm going to merge this if there's no further comment.

@tengqm
Copy link
Contributor

tengqm commented Nov 27, 2024

no objection from me

@jerryshao jerryshao merged commit 478b6ea into apache:main Nov 28, 2024
26 checks passed
@orenccl orenccl deleted the improvement/jarName branch December 22, 2024 09:25
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.

[Improvement] Update the jar name to add gravitino prefix
3 participants