-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
[feature][tool] Add the Maven wrapper #10487
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.
very good
Do we have to add .mvn\wrapper in .gitignore ?
otherwise someone may end in committing the compiled wrapper.jar ?
The Maven binary is downloaded into the |
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 |
Would it make sense to cache the downloaded Maven binary with GHA cache? |
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
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.
@lhotari
There is no need to use mvnw in CI.
btw we could enhance CI as a separate step.
@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 there would be a benefit of using
that's true. |
README.md
Outdated
@@ -93,32 +93,32 @@ Requirements: | |||
Compile and install: | |||
|
|||
```bash | |||
$ mvn install -DskipTests | |||
$ ./mvnw install -DskipTests |
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.
does this script works for Windows environment?
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.
On Windows it's the mvn.cmd script that shall be used.
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.
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.
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.
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 './')
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.
Is someone able to test it on Windows ?
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.
On Windows, ./mvnw
won't work on standard terminal. You need to either use mvnw
or .\mvnw
/pulsarbot run-failure-checks |
I changed the README to keep |
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.
.mvn/wrapper/maven-wrapper.jar
Per ASF policy, we don't recommend adding binary into the source code repo. This should be removed.
done |
I believe this could be merged ? |
@codelipenghui is releasing 2.8.0. will merge it once 2.8.0 is done. |
Hi, can this have another shot now that 2.8 is out ? |
Hi, can this be merged ? |
@sijie @codelipenghui it would be good to merge this before 2.9.0 release |
Hi, can this be merged ? |
@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 |
@codelipenghui @sijie mentioned you in this thread. also @sijie can you please update your "Request for changes" review if you think it is time to go with this patch |
Hi. Can this be merged or closed ? |
This patch is very old, it need re-evaluating
8d1f212
to
ab38c2d
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.
LGTM
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.
@cbornet - it's probably also worth updating the contributor's guide: https://pulsar.apache.org/contributing/.
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. 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.
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. 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.
@cbornet:Thanks for your contribution. For this PR, do we need to update docs? |
I believe that this change is harmless and we can commit to every active branch (up to to 2.8) |
Motivation
Modifications
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
This change is a trivial rework
Does this pull request potentially affect one of the following parts:
If
yes
was chosen, please highlight the changesDocumentation