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

[MINOR] refactor: Tweak the build script #6677

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

tengqm
Copy link
Contributor

@tengqm tengqm commented Mar 12, 2025

What changes were proposed in this pull request?

This PR tweaks the build script for coding style consistency.
It also fixes a few nits in the scripts:

  • duplicated entries caused by unsorted items
  • incorrect dependency items missing leading ":"

Why are the changes needed?

See description.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

N/A

@jerryshao
Copy link
Contributor

@tengqm There're some build issues about the change, can you please check the CI log and fix it.

@tengqm
Copy link
Contributor Author

tengqm commented Mar 12, 2025

@tengqm There're some build issues about the change, can you please check the CI log and fix it.

OK.

@tengqm tengqm force-pushed the fix-build-script branch 3 times, most recently from 2b65d42 to eab465d Compare March 12, 2025 13:21
"--add-opens", "java.base/sun.nio.ch=ALL-UNNAMED",
"--add-opens", "java.base/sun.nio.cs=ALL-UNNAMED",
"--add-opens", "java.base/sun.security.action=ALL-UNNAMED",
"--add-opens", "java.base/sun.util.calendar=ALL-UNNAMED",
"--add-opens", "java.security.jgss/sun.security.krb5=ALL-UNNAMED",
"--add-opens", "java.base/java.lang.reflect=ALL-UNNAMED"
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 reviwers: This entry was found to be duplicated after sorting the list.

"--add-opens", "java.base/jdk.internal.ref=ALL-UNNAMED",
"--add-opens", "java.base/jdk.internal.reflect=ALL-UNNAMED",
"--add-opens", "java.sql/java.sql=ALL-UNNAMED",
"--add-opens", "java.base/sun.util.calendar=ALL-UNNAMED",
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 reviwers: This entry was found to be duplicated after sorting the list.

Copy link
Contributor

Choose a reason for hiding this comment

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

good catch.

"web/web/src/lib/icons/svg/**/*.svg",
"web/web/src/lib/utils/axios/**/*",
"web/web/src/types/axios.d.ts",
"web/web/yarn.lock"
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 reviwers: contents not changed other than sorting the entries

"copySubprojectDependencies",
"copySubprojectLib",
":authorizations:copyLibAndConfig",
":iceberg:iceberg-rest-server:copyLibAndConfigs",
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 reviwers: by wrapping the long line, the missing leading ':' for iceberg one becomes obvious.

it.parent?.name != "bundles" && it.name != "hadoop-common"
!it.name.startsWith("client") &&
!it.name.startsWith("filesystem") &&
!it.name.startsWith("flink") &&
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 reviwers: by wrapping the long line, we found that 'flink' is duplicated.

it.name != "bundled-catalog" &&
it.name != "hadoop-common" &&
it.name != "hive-metastore-common" &&
it.name != "integration-test" &&
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 reviwers: by wrapping the long line and sorting the entries, duplication of 'integration-test' is obvious.

":catalogs:catalog-kafka:copyLibAndConfig",
":catalogs:catalog-lakehouse-hudi:copyLibAndConfig",
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 reviewers: 'catalogs:catalog-lakehouse-hudi:copyLibAndConfig' was missing the leading ':'.

Copy link
Contributor

Choose a reason for hiding this comment

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

�It's fine, these two ways are both workable for Gradle.

@tengqm tengqm force-pushed the fix-build-script branch from eab465d to b87b73c Compare March 13, 2025 01:35
@jerryshao jerryshao requested a review from yuqi1129 March 14, 2025 02:45
@jerryshao
Copy link
Contributor

@yuqi1129 can you please help to review this PR?

"--add-opens", "java.base/jdk.internal.ref=ALL-UNNAMED",
"--add-opens", "java.base/jdk.internal.reflect=ALL-UNNAMED",
"--add-opens", "java.sql/java.sql=ALL-UNNAMED",
"--add-opens", "java.base/sun.util.calendar=ALL-UNNAMED",
Copy link
Contributor

Choose a reason for hiding this comment

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

good catch.

@tengqm tengqm force-pushed the fix-build-script branch from b87b73c to 4abc291 Compare March 14, 2025 03:47
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.

3 participants