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

[BuildTools][Java] use maven wrapper in every project instead of requiring contributor to install maven in his environment #336

Closed
bushi-go opened this issue May 31, 2024 · 4 comments · Fixed by green-code-initiative/ecoCode-java#67
Assignees
Labels
🔰 good first issue Good for newcomers 🏗️ refactoring refactoring for best practices 🚀 enhancement New feature or request

Comments

@bushi-go
Copy link

bushi-go commented May 31, 2024

Is your feature request related to a problem? Please describe.

Hi ! I contributed to the project for the first time during the 2024 ecocode challenge.

While setting up my dev env, i had to install maven (among other things) by hand, as i do no longer use it, having joined the gradle band wagon ;p

Having to install maven "by hand" is a bit cumbersome ; even more so when it must be installed within a specific range of version.

It can be a bit frustrating for people using maven but at a version that is not within that range.

All things being equals it causes, in my opinion, a mild degradation in the developper experience for new contributors, and a very avoidable one. ;)

Describe the solution you'd like

We could use the maven wrapper tool in every java project of the org :)
You can generate it with the following command : mvn wrapper:wrapper and commit the generated files in each repo

Expected benefits :

  • it is a standard tool, proposed by the two main build tools of the java community (gradle having its own wrapper, but following the same conventions)
  • no need for the users to install maven by themselve ; the maven wrapper script will do it for them by itself and keep it local to the project. It will ease the onboarding process, by removing a point of friction
  • the version is configurable in the maven-wrapper.properties, which may simplify maintenance
  • It could have benefits for projects CI, as we won't need an image with maven already installed
  • It is compatible with both windows and linux env, as the script comes with a .sh and a .bat version
  • Most IDE will detect and use the maven-wrapper if it is present, or can be instructed to do so instead of using a local or embedded install.

Drawbacks

  • It may impact negatively the CI, since without a proper caching in place the maven binary will be dowloaded at each run

Describe alternatives you've considered
One can still use sdkman to manage multiple maven version by oneself, but the burden of setup still lies with the contributor

Additional Context

@jycr jycr self-assigned this May 31, 2024
@dedece35
Copy link
Member

dedece35 commented Jun 2, 2024

Hello @bushi-go,

thank you for idea. For me it's a good idea.
Indeed, actual requirements (here) are the minimum technical stack to contribute to ecocode projects.
I agree with you that anyone who wants to contribute must install these frameworks / tools, even if he/she isn't used to using them.
We are always looking for improvements for everything that's why it's a good idea for me.
We also discussed with @jycr on improvements for docker and docker-compose, to avoid them because of some security constraints among some societies.
For JDK and GIT, I don't know if we can do something to avoid them.
Yes, solutions like "sdkman.io" allow us to work with different versions.
Regarding versions problem, we have some ranges because we didn't test new versions yet and we can't guarantee the good functionning of latest versions.

Feel free to work on your idea and create a PR for it.
Maybe @jycr, do you want to work on it (you assigned this issue to yourself) ?

@dedece35 dedece35 added 🚀 enhancement New feature or request 🔰 good first issue Good for newcomers 🏗️ refactoring refactoring for best practices labels Jun 2, 2024
@jycr
Copy link
Contributor

jycr commented Jun 2, 2024

@dedece35 : Yes, I can work on it.
But I want to finish some PR/issues before working on this one, like :

@glalloue
Copy link
Contributor

glalloue commented Jun 3, 2024

I think it's a good idea @bushi-go , and it will improve the first contribution experience.
Feel free to start working on it or to work with @jycr on this subject
You can also synchronize on our slack to discuss this subject.
Thanks for your contribution ;)

@bushi-go
Copy link
Author

bushi-go commented Jun 3, 2024

Hello @dedece35, @jycr, @glalloue,

I will be more than happy to contribute the PR's to address this. @jycr we can sync up at your convenience, i will ping you on slack. I have a good half of the code already done on my forks so i can keep working my way through it if you'd prefer keeping with your other issues :)

Just one thing though : that might force people to upgrade their JDK to a version compatible with the maven dist defined in the wrapper property file. (JDK-17 i think at the time). It should not be a big deal in most cases :)

@dedece35, about the other points you made :

  • On git and jdk, obviously they are mandatory requirements and can't be avoided. Maybe we could point people to the installation docs of the various tools to save them a google search in the check_requirements.sh output ? Or provide ourselves an install/setup script ?
  • on the docker part, to be fair, it is only needed as part of the integration phase. I think it is rather sane to ask people to test that way. The overral process is pretty good as it is, but a bit tedious because of some manual steps that remains (like generating the token and all). I don't know if we could automate it but that would probably be a nice enhancement

I might try a thing or two and open issues about them to discuss further

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔰 good first issue Good for newcomers 🏗️ refactoring refactoring for best practices 🚀 enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants