-
Notifications
You must be signed in to change notification settings - Fork 40
Fix build error in data-loader #2757
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
Conversation
12d6af7 to
1a8a8c7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR addresses a build error in the data-loader module by attempting to fix task ordering between shadowJar and its dependent tasks.
- Adds a mustRunAfter clause to shadowJar for distZip and distTar tasks.
- Aims to resolve Gradle configuration issues related to task dependencies.
|
|
||
| // Build a fat jar | ||
| shadowJar { | ||
| mustRunAfter distZip, distTar |
Copilot
AI
Jun 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The 'mustRunAfter distZip, distTar' statement forces shadowJar to execute after distZip and distTar, which contradicts the intended dependency flow since these tasks depend on shadowJar's output. Consider instead having distZip, distTar, and startScripts declare an explicit dependency on shadowJar using 'dependsOn' to ensure the correct execution order.
| mustRunAfter distZip, distTar |
ypeckstadt
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thank you.
@komamitsu I ran |
|
@brfrn169 Thank you for finding this one. This is really something that should not have happened as it's the basic build command. To avoid issues like this, we will need to add the data loader to the CI workflows. If ok, I will create a PR to add it. Is it best to have a separate workflow file or it can be added to the CI.yaml one? |
Torch3333
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thank you!
@ypeckstadt You can create a separate workflow file for the data-loader CI. |
komamitsu
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thank you!
Honestly speaking, https://github.com/scalar-labs/scalardb/pull/2757/files#r2141443330 sounds reasonable to me. But, I think there is no dependency between the distribution tasks and the shadow jar task, so explicitly defining the dependency somehow should be a simple solution for the issue.
feeblefakie
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thank you!
|
@inv-jishnu Can you please check this PR? |
inv-jishnu
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Thank you.
Description
I encountered the following error when building the
data-loadermodule locally:According to the error messages, we need to declare explicit dependencies between the shadowJar task and some other to ensure correct execution order.
This PR fixes the issue in the
data-loadermodule by specifying the necessary task dependencies.Related issues and/or PRs
N/A
Changes made
Checklist
Additional notes (optional)
N/A
Release notes
N/A