joular / joularjx

JoularJX is a Java-based agent for software power monitoring at the source code level.
https://www.noureddine.org/research/joular/joularjx
GNU General Public License v3.0
71 stars 17 forks source link

Add Maven Wrapper Support #16

Closed karianna closed 1 year ago

karianna commented 1 year ago

See #15 - This allows users not to worry about having a separate up to date Maven install.

adelnoureddine commented 1 year ago

Thanks, I'll have a look at it.

adelnoureddine commented 1 year ago

Thanks for the suggestion and the PR. Unfortunately, I won't integrate this feature. Although I see the benefit for users, in particular to streamline installation, but it adds additional weight and code that is constantly being downloaded even when users have maven installed. This is quite problematic for lighter embedded systems, and the target users have probably maven already installed. We do provide with a compiled JAR in our repo, so this should mitigate the issue and users can use JoularJX without compiling it or using Maven.

aalmiray commented 1 year ago

This is quite problematic for lighter embedded systems, and the target users have probably maven already installed.

Pardon me, but is it a goal for this project to build and run joularjx.jar in an embedded system? It would be enough to build on any Linux compatible platform and then ship the binaries to the embedded system. curl + unzip on the target system is better than installing maven and build IMHO.

For this reason having the wrapper available for builders makes more sense. Pre-building binaries for consumers (as you do today) enables more people to consume the project without having to build it first.

adelnoureddine commented 1 year ago

Thanks, yes you are right. However, I'm still not quite convinced for the added value, as users will still need to have a Java compiler and JDK installed to even use the shipped maven version. I'll reopen the issue, so we can collect more opinions and check similar projects' usage before making a definitive decision.

aalmiray commented 1 year ago

Thank you for resuming the discussion on the wrapper. I can offer this post to clear out some of the concepts https://andresalmiray.com/why-do-the-gradle-maven-wrappers-matter/

One would expect builders have a version of Java and Maven capable of building the project. But, are those the correct versions?

Builders can leverage the Maven wrapper to bootstrap mvn. This project may leverage the wrapper to ensure the correct/minimum version of Maven is used to build the project. At the moment there's no Java wrapper/bootstrap tool, the expectation of having a JDK readily available before building remains.

However the maven enforcer plugin may be used to check the Java version and Maven version, failing the build when there's no match. I'd also recommend having s look at the requireJavaVersion and requireMavenVersion rules. Why the latter? Because even if the wrapper were to be added to the project a builder might opt to not use it or miss it, probably resulting in the wrong version of Maven for the build.

There are other items that could be discussed to make the build more resilient and hopefully reproducible, but I'd like to make a pause for the moment and start with just the first step: the maven wrapper.

karianna commented 1 year ago

Happy to resubmit the PR if the maintainers choose to have it, but it's their project :-)

adelnoureddine commented 1 year ago

Thanks for all your input. I've added maven wrapper support in commit faf26b795532eeec90cb565f346e05f6f0e12153 (generated with mvn wrapper:wrapper).

aalmiray commented 1 year ago

Thanks for all your input. I've added maven wrapper support in commit faf26b7 (generated with mvn wrapper:wrapper).

🎉 FWIW latest Maven release is 3.9.0.

adelnoureddine commented 1 year ago

Thanks, it's updated!