[feature][tool] Add the Maven wrapper #10487

cbornet · 2021-05-05T13:42:39Z

Motivation

The maven wrapper helps getting started quickly without having to install Maven
It helps to control that a working version of Maven is used

Modifications

Added the wrapper
Modified READMEs to use the wrapper

Scripts and CI still use the unwrapped mvn command and Maven is still indicated as a requirement but this could be changed in a later PR.

Verifying this change

Install the project with ./mvnw install -DskipTests

Make sure that the change passes the CI checks.

This change is a trivial rework

Does this pull request potentially affect one of the following parts:

If yes was chosen, please highlight the changes

Dependencies (does it add or upgrade a dependency): (yes / no) no
The public API: (yes / no) no
The schema: (yes / no / don't know) no
The default values of configurations: (yes / no) no
The wire protocol: (yes / no) no
The rest endpoints: (yes / no) no
The admin cli options: (yes / no) no
Anything that affects deployment: (yes / no / don't know) no

Documentation

Does this pull request introduce a new feature? (yes / no) yes
If yes, how is the feature documented? (not applicable / docs / JavaDocs / not documented) in READMEs
If a feature is not applicable for documentation, explain why?
If a feature is not documented yet in this PR, please create a followup issue for adding the documentation

eolivelli

very good

Do we have to add .mvn\wrapper in .gitignore ?
otherwise someone may end in committing the compiled wrapper.jar ?

cbornet · 2021-05-05T14:26:27Z

The Maven binary is downloaded into the ~/.m2 directory so there's no need to add it to gitignore

lhotari · 2021-05-05T14:57:58Z

Now that maven 3.8.1 is used, it should be possible to remove the hack which replaces maven's wagon-http jar file. That solution is in build/replace_maven-wagon-http-version.sh script. The script is referenced from the GHA workflows in .github/workflows.

lhotari · 2021-05-05T14:59:20Z

Would it make sense to cache the downloaded Maven binary with GHA cache?

eolivelli

LGTM

eolivelli

@lhotari
There is no need to use mvnw in CI.

btw we could enhance CI as a separate step.

eolivelli · 2021-05-05T15:46:46Z

@cbornet thank you for providing this patch.

before merging this kind of patches it is better to start a discussion on dev@pulsar and see what is the feeling from the community

eolivelli · 2021-05-05T15:48:54Z

@merlimat @rdhabalia @codelipenghui @sijie FYI

lhotari · 2021-05-05T16:17:59Z

There is no need to use mvnw in CI.

@eolivelli there would be a benefit of using mvnw since we could get rid of the wagon-http upgrade hack which is required in CI. mvnw is able to pull a specific maven version, which is 3.8.1 that contains the required fixes to wagon-http.

btw we could enhance CI as a separate step.

that's true.

.mvn/wrapper/MavenWrapperDownloader.java

sijie · 2021-05-05T21:23:30Z

README.md

@@ -93,32 +93,32 @@ Requirements:
 Compile and install:

 ```bash
-$ mvn install -DskipTests
+$ ./mvnw install -DskipTests


does this script works for Windows environment?

On Windows it's the mvn.cmd script that shall be used.

How do people know to use mvn.cmd?

If people use its local maven installation, mvn install ... works across different operating systems.

I am not against adding the mvn wrapper. But I don't think we should change the documentation. Once you change the documentation, you make people using windows hard to get started on building pulsar.

you can still use class "mvn" command.
you are not forced to use mvnw.

IIRC on Windows when you have "mvnw.cmd" you can use "mvnw" as command, like for .bat files

so the usage is the same on Linux/Mac/Windows, it is always "./mvnw". (probably on windows you can also omit './')

Is someone able to test it on Windows ?

On Windows, ./mvnw won't work on standard terminal. You need to either use mvnw or .\mvnw

cbornet · 2021-05-12T09:06:33Z

/pulsarbot run-failure-checks

cbornet · 2021-05-12T14:11:26Z

I changed the README to keep mvn in the command sample and added a note on how to use the wrapper.
cc @sijie

sijie

.mvn/wrapper/maven-wrapper.jar

Per ASF policy, we don't recommend adding binary into the source code repo. This should be removed.

cbornet · 2021-05-17T08:48:45Z

done

cbornet · 2021-06-03T08:34:25Z

I believe this could be merged ?

sijie · 2021-06-03T08:44:29Z

@codelipenghui is releasing 2.8.0. will merge it once 2.8.0 is done.

cbornet · 2021-07-22T15:43:02Z

Hi, can this have another shot now that 2.8 is out ?

cbornet · 2021-08-26T14:43:53Z

Hi, can this be merged ?

eolivelli · 2021-08-31T15:29:33Z

@sijie @codelipenghui it would be good to merge this before 2.9.0 release

cbornet · 2021-11-10T16:07:54Z

Hi, can this be merged ?

eolivelli · 2021-11-10T16:23:37Z

@cbornet thanks for this useful work.

If there is no discussion on the dev@ list please start a new thread or ping on the existing thread.

here you will have a smaller audience

eolivelli · 2021-11-10T16:27:55Z

@codelipenghui @sijie mentioned you in this thread.
it is time to commit this patch ?

also @sijie can you please update your "Request for changes" review if you think it is time to go with this patch

cbornet · 2022-04-10T17:09:13Z

Hi. Can this be merged or closed ?

This patch is very old, it need re-evaluating

.mvn/wrapper/maven-wrapper.properties

README.md

.mvn/wrapper/maven-wrapper.properties

lhotari

LGTM

michaeljmarshall

@cbornet - it's probably also worth updating the contributor's guide: https://pulsar.apache.org/contributing/.

merlimat

LGTM. We should also update the CI to use mvnw so that we standardize on 1 version of maven. We have seen that there are some small changes in the dependencies list depending on the maven version.

michaeljmarshall

LGTM. Is there any reason we shouldn't cherry pick to this to active maintenance branches? It seems like it'd ensure a more consistent environment when releasing new versions, which would be valuable.

github-actions · 2022-05-13T17:03:47Z

@cbornet:Thanks for your contribution. For this PR, do we need to update docs?
(The PR template contains info about doc, which helps others know more about the changes. Can you provide doc-related info in this and future PR descriptions? Thanks)

eolivelli · 2022-05-16T07:01:37Z

LGTM. Is there any reason we shouldn't cherry pick to this to active maintenance branches? It seems like it'd ensure a more consistent environment when releasing new versions, which would be valuable.

I believe that this change is harmless and we can commit to every active branch (up to to 2.8)

eolivelli requested changes May 5, 2021

View reviewed changes

eolivelli previously approved these changes May 5, 2021

View reviewed changes

eolivelli reviewed May 5, 2021

View reviewed changes

eolivelli requested review from merlimat and rdhabalia May 5, 2021 15:48

sijie requested changes May 5, 2021

View reviewed changes

cbornet force-pushed the mvn-wrapper branch from 7e0f1f2 to 3d0a670 Compare May 11, 2021 15:36

sijie previously requested changes May 15, 2021

View reviewed changes

cbornet requested a review from sijie May 18, 2021 18:43

congbobo184 dismissed eolivelli’s stale review via 755b745 February 16, 2022 02:58

eolivelli approved these changes Apr 20, 2022

View reviewed changes

nicoloboschi approved these changes May 13, 2022

View reviewed changes

lhotari requested changes May 13, 2022

View reviewed changes

.mvn/wrapper/maven-wrapper.properties Outdated Show resolved Hide resolved

README.md Outdated Show resolved Hide resolved

.mvn/wrapper/maven-wrapper.properties Outdated Show resolved Hide resolved

cbornet force-pushed the mvn-wrapper branch 2 times, most recently from 8d1f212 to ab38c2d Compare May 13, 2022 12:44

Add the Maven wrapper

4f66ee0

cbornet force-pushed the mvn-wrapper branch from ab38c2d to 4f66ee0 Compare May 13, 2022 12:47

lhotari approved these changes May 13, 2022

View reviewed changes

michaeljmarshall reviewed May 13, 2022

View reviewed changes

dlg99 approved these changes May 13, 2022

View reviewed changes

merlimat added the area/build label May 13, 2022

merlimat approved these changes May 13, 2022

View reviewed changes

michaeljmarshall approved these changes May 13, 2022

View reviewed changes

github-actions bot assigned cbornet May 13, 2022

github-actions bot added the doc-label-missing label May 13, 2022

michaeljmarshall added doc Your PR contains doc changes, no matter whether the changes are in markdown or code files. and removed doc-label-missing labels May 13, 2022

cbornet changed the title ~~Add the Maven wrapper~~ [feature][tool] Add the Maven wrapper May 21, 2022

eolivelli merged commit ec9237a into apache:master May 23, 2022

cbornet deleted the mvn-wrapper branch May 23, 2022 12:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[feature][tool] Add the Maven wrapper #10487

[feature][tool] Add the Maven wrapper #10487

cbornet commented May 5, 2021 •

edited

eolivelli left a comment

cbornet commented May 5, 2021

lhotari commented May 5, 2021

lhotari commented May 5, 2021

eolivelli left a comment

eolivelli left a comment

eolivelli commented May 5, 2021

eolivelli commented May 5, 2021

lhotari commented May 5, 2021

sijie May 5, 2021

cbornet May 5, 2021

sijie May 6, 2021

eolivelli May 6, 2021

cbornet May 6, 2021

cbornet May 7, 2021

cbornet commented May 12, 2021

cbornet commented May 12, 2021

sijie left a comment

cbornet commented May 17, 2021

cbornet commented Jun 3, 2021

sijie commented Jun 3, 2021

cbornet commented Jul 22, 2021

cbornet commented Aug 26, 2021

eolivelli commented Aug 31, 2021

cbornet commented Nov 10, 2021

eolivelli commented Nov 10, 2021

eolivelli commented Nov 10, 2021

cbornet commented Apr 10, 2022

lhotari left a comment

michaeljmarshall left a comment

merlimat left a comment

michaeljmarshall left a comment

github-actions bot commented May 13, 2022

eolivelli commented May 16, 2022

[feature][tool] Add the Maven wrapper #10487

[feature][tool] Add the Maven wrapper #10487

Conversation

cbornet commented May 5, 2021 • edited

Motivation

Modifications

Verifying this change

Does this pull request potentially affect one of the following parts:

Documentation

eolivelli left a comment

Choose a reason for hiding this comment

cbornet commented May 5, 2021

lhotari commented May 5, 2021

lhotari commented May 5, 2021

eolivelli left a comment

Choose a reason for hiding this comment

eolivelli left a comment

Choose a reason for hiding this comment

eolivelli commented May 5, 2021

eolivelli commented May 5, 2021

lhotari commented May 5, 2021

sijie May 5, 2021

Choose a reason for hiding this comment

cbornet May 5, 2021

Choose a reason for hiding this comment

sijie May 6, 2021

Choose a reason for hiding this comment

eolivelli May 6, 2021

Choose a reason for hiding this comment

cbornet May 6, 2021

Choose a reason for hiding this comment

cbornet May 7, 2021

Choose a reason for hiding this comment

cbornet commented May 12, 2021

cbornet commented May 12, 2021

sijie left a comment

Choose a reason for hiding this comment

cbornet commented May 17, 2021

cbornet commented Jun 3, 2021

sijie commented Jun 3, 2021

cbornet commented Jul 22, 2021

cbornet commented Aug 26, 2021

eolivelli commented Aug 31, 2021

cbornet commented Nov 10, 2021

eolivelli commented Nov 10, 2021

eolivelli commented Nov 10, 2021

cbornet commented Apr 10, 2022

lhotari left a comment

Choose a reason for hiding this comment

michaeljmarshall left a comment

Choose a reason for hiding this comment

merlimat left a comment

Choose a reason for hiding this comment

michaeljmarshall left a comment

Choose a reason for hiding this comment

github-actions bot commented May 13, 2022

eolivelli commented May 16, 2022

cbornet commented May 5, 2021 •

edited