-
Notifications
You must be signed in to change notification settings - Fork 149
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] improvement: use mvn wrapper for builds #1345
Conversation
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.
We should not include maven jar file, it can be downloaded by the script if missing.
Also consider use a newer version of maven? (3.9.6 is available)
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1345 +/- ##
============================================
- Coverage 53.28% 53.26% -0.02%
Complexity 2712 2712
============================================
Files 418 418
Lines 23901 23901
Branches 2042 2042
============================================
- Hits 12735 12732 -3
- Misses 10381 10384 +3
Partials 785 785 ☔ View full report in Codecov by Sentry. |
I've included it initially as it seems to be the standard/recommended practice (that's what the wrapper plugin defaults to). Having said that, it does have the option to opt-out of additional files which I've used to regenerate the wrapper files. I've also added the jar to
For context - the last time I checked the latest version of @kaijchen FYI |
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, thanks for the update @lukhar.
We can use mvnw
in the CI scripts to ensure it's always correct (can be changed in a later PR)
### What changes were proposed in this pull request? This is a follow-up to #1345. The change will (hopefully) invoke `./mvnw` (wrapper) instead of the `mvn` command in CI builds, making the local development builds and CI builds equivalent to the applied `maven` version. ### Why are the changes needed? It's prudent to have the development builds as close to the ones executed by CI/CD system as possible. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? I believe having a PR builder pass should suffice.
What changes were proposed in this pull request?
Introducing maven wrapper (version:
3.6.3
3.9.6
). While working on building and deployinguniffle
it took me considerably long to pin the right version of maven. Looking at: #685 it seems that I wasn't the only person who had this problem (the service docs aren't clear about which maven version works). I think that introducing maven wrapper and pinning its version to the working configuration will reduce the friction for new contributors and users of the shuffle service.Why are the changes needed?
Pinning down the maven version reduces friction for the project setup (one doesn't need to ensure a specific version of maven is installed) and builds stability (the build is more resilient to changes in the build environments).
Fix: #685
Does this PR introduce any user-facing change?
We're moving from
mvn
to./mvw
command to execute build-related tasks.How was this patch tested?
I've ensured that:
build.sh
andbuild_distribution.sh
build validuniffle
distribution./mvnw*
commands listed in the README files are executed without errors.