Skip to content

[MINOR] improvement: use mvn wrapper for builds - #1345

Merged
kaijchen merged 3 commits into
apache:masterfrom
lukhar:use-mvn-wrapper
Dec 5, 2023
Merged

[MINOR] improvement: use mvn wrapper for builds#1345
kaijchen merged 3 commits into
apache:masterfrom
lukhar:use-mvn-wrapper

Conversation

@lukhar

@lukhar lukhar commented Dec 4, 2023

Copy link
Copy Markdown
Contributor

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

@kaijchen kaijchen left a comment

Copy link
Copy Markdown
Member

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

codecov-commenter commented Dec 5, 2023

Copy link
Copy Markdown

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

lukhar commented Dec 5, 2023

Copy link
Copy Markdown
Contributor Author

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

@kaijchen kaijchen left a comment

Copy link
Copy Markdown
Member

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
@lukhar
lukhar deleted the use-mvn-wrapper branch December 6, 2023 03:35
@lukhar

lukhar commented Dec 6, 2023

Copy link
Copy Markdown
Contributor Author

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