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] improvement: use mvn wrapper for builds #1345

Merged
merged 3 commits into from
Dec 5, 2023

Conversation

lukhar
Copy link
Contributor

@lukhar lukhar commented Dec 4, 2023

What changes were proposed in this pull request?

Introducing maven wrapper (version: 3.6.3 3.9.6). While working on building and deploying uniffle 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 and build_distribution.sh build valid uniffle distribution
  • all ./mvnw* commands listed in the README files are executed without errors.

@zuston zuston requested a review from kaijchen December 5, 2023 01:52
Copy link
Contributor

@kaijchen kaijchen left a 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-commenter
Copy link

codecov-commenter commented Dec 5, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (e3361d6) 53.28% compared to head (1c93b86) 53.26%.

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.
📢 Have feedback on the report? Share it here.

@lukhar
Copy link
Contributor Author

lukhar commented Dec 5, 2023

We should not include maven jar file, it can be downloaded by the script if missing.

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 .gitignore to ensure it won't get committed by accident. I've left the properties file in .mvn/wrapper directory should we need to adjust the configuration in the future. Let me know if that's acceptable.

Also consider use a newer version of maven? (3.9.6 is available)

For context - the last time I checked the latest version of maven wasn't able to build the project (issues were similar to the ones mentioned #685). The 3.6.3 was recommended in the issue and that's what I've pinned my environment to when working on the project. I've checked 3.9.6 and looks like it works as well. The PR was updated accordingly.

@kaijchen FYI

Copy link
Contributor

@kaijchen kaijchen left a 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)

@kaijchen kaijchen merged commit eef0134 into apache:master Dec 5, 2023
36 checks passed
@lukhar lukhar deleted the use-mvn-wrapper branch December 6, 2023 03:35
@lukhar
Copy link
Contributor Author

lukhar commented Dec 6, 2023

We can use mvnw in the CI scripts to ensure it's always correct (can be changed in a later PR)

I've made an attempt to make this happen: #1351. Rather new to GitHub workflows, so I'm not entirely sure whether I've covered everything. Any feedback on this would be appreciated.

@kaijchen FYI

kaijchen pushed a commit that referenced this pull request Dec 6, 2023
### 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.
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.

[Bug] Probloms of RequireUpperBoundDeps
3 participants