patric-r / jvmtop

Java monitoring for the command-line, profiler included
GNU General Public License v2.0
1.22k stars 252 forks source link

mavenizing jvmtop #66

Closed JigarJoshi closed 8 years ago

JigarJoshi commented 9 years ago

mavenizing jvmtop #65

patric-r commented 9 years ago

JigarJoshi, thank you so much for the PR. I'll take a look. I noticed that you deleted the eclipse project settings. Is there a way to integrate them into a mavenized project?

JigarJoshi commented 9 years ago

I could re-add it, usually maven projects are IDE independent, was the purpose of .settings to enforce coding style pattern ?

patric-r commented 9 years ago

Yes, exactly - and (for the .project and .classpath files) to be able to easily checkout and start using the project in eclipse without the need invoking maven beforehand (or requiring m2eclipse & friends).

JigarJoshi commented 9 years ago

with maven, you would need m2eclipse if you want to work with eclipse, to manage classpath by reading pom.xml and interpreting dependencies, I can add .settings back, .project and .classpath can remain out of it, you could still just import project in eclipse with m2eclipse

what do you think ?

JigarJoshi commented 9 years ago

Hi Thanks @patric-r , Would you be merging it ?

patric-r commented 8 years ago

@JigarJoshi I checked out your branch using m2eclipse. Unfortunately, it does not compile out of the box because of the tools.jar dependency. Most probably I've to adjust the pom.xml to point to the correct directory where this jar can be located.

Isn't it possible to resolve the tools.jar dependency automatically like described here?

Currently, builds cannot be made without additionally effort (which was possible beforehand).

JigarJoshi commented 8 years ago

I am using the same approach, which operating system you tried this on ?

patric-r commented 8 years ago

I tried this using m2eclipse on windows 7.

axelhodler commented 8 years ago

just tried this on osx, works

@patric-r

if you type

mvn -version

where does the Java home point to? (it is used in pom.xml as ${java.home})

patric-r commented 8 years ago

@axelhodler I tried this using the most recent m2eclipse, not the command-line mvn application (where most probably everything will work correctly). I'll let you know tomorrow if there's a similar view inside m2eclipse to print the information you requested.

JigarJoshi commented 8 years ago

Yes there is huge difference between command line and IDE supports plugin in terms of stability, I will try it on windows environment soon and update here

patric-r commented 8 years ago

Using m2eclipse "Effective POM" view, I can see that the expression ${java.home}/../lib/tools.jar evaluates to C:\Program Files (x86)\Java\jre1.8.0_60/../lib/tools.jar on my machine, which is non-existent and hence wrong.
Assuming that ${java.home} points to a JDK base directory, which is what I would expect, ${java.home}/lib/tools.jar would be the correct expression.

But, unfortunately, even if changing the expression to the correct path, m2eclipse keeps showing the same build errors - for some reasons, it seems to ignore the (now) valid path to tools.jar and keeps referencing to the local m2 repository:

Description Resource Path Location Type
Missing artifact com.sun:tools:jar:1.6.0 pom.xml /jvmtop-joshi line 17 Maven Dependency Problem
The container 'Maven Dependencies'
references non existing library C:\Users\myuser\.m2\repository\ com\sun\tools\1.6.0\tools-1.6.0.jar
jvmtop-joshi Build path Build Path Problem
The project cannot be built until build path errors are resolved jvmtop-joshi Unknown Java Problem
axelhodler commented 8 years ago

{java.home} should point to the JRE folder inside of the JDK folder, so the ../lib/tools.jar is correct

i guess something is wrong with the maven install if it references non existing folders

JigarJoshi commented 8 years ago

$JAVA_HOME should point to JDK's location, I added a tweak to support both, I do not have access to windows setup at this moment, I verified it works on mac, could you please reevaluate it now

patric-r commented 8 years ago

@JigarJoshi I think I figured out the main cause of my problem: My m2eclipse test installation (which differs from my daily use IDE) used a JRE as the default runtime. However, tools.jar is not part of any JRE at all (a JDK is required). I'm sorry for wasting your time, I should have noticed this earlier.

I think we can now go back to the previous version of your branch without any OS-specific profiles and where only ${java.home} was used ( $JAVA_HOME is not guaranteed to be set on a windows installation).

The only thing which is left, are the missing eclipse coding style/compiler settings. This plugin looks promising - what do you think?

JigarJoshi commented 8 years ago

no worries, I reverted pom.xml back to that revision

about code style: I don't think it is good idea to make a maven project eclipse specific for the purpose of code style linting there is a maven-checkstyle-plugin that sounds more promising to me, I personally haven't used it, There are 2 standard check style published already from Sun & Google, We could use it to guard on top of manual eyes, Thoughts ?

patric-r commented 8 years ago

Thank you.

I'm unsure about maven-checkstyle-plugin and it's IDE integration. IMHO, code formatting settings which apply automatically e.g. on each save action are much more integrated in the development process than just scanning for violations and output them to the console (or build breaking) like maven-checkstyle-plugin seems to do.

However, in order to continue I suggest merging your PR soon but afterwards I'll add exported eclipse code formatter settings to the root directory. Everyone is free to important this file to his eclipse project. If this works without causing code style mixups, I'll be happy.

Some additional questions: Maven artifact com.sun.tools is defined in pom.xml as version 1.6.0. What effect has this version number on the build process (the tools.jar will be read from local file system in any way)? What happens if JDKs from higher versions are used or if the version in the pom.xml would be increased?

JigarJoshi commented 8 years ago

I don't know the effect of version in here, may be some plugins would be using it for some dependencies related processing

Re: JDK's version and dependency's version, I just tied them up (compiler and system dependency's version here)

patric-r commented 8 years ago

Ok - I was just asking - not complaining about the value 1.6.0 which I think is better than 1.8.0 I think we now need to change pom.xml to define <jdk.version>1.6</jdk.version> instead. Otherwise, the created jarfile cannot be run on JDKs below release 1.8.

After this has been changed, I think we're ready to merge your branch into mine.

JigarJoshi commented 8 years ago

I think we should put lower threshold to 8 or in the worst case 7, 6 is too old, you might have better knowledge of where are the major users, what do you think ?

patric-r commented 8 years ago

We should not forget: Oracle's Java 6 extended support lasts until the end of 2018, there are still many legacy applications which are built on top of java 6.

Because jvmtop is often used in professional environments of those applications, I don't want to exclude JDK 6 users right now.

JigarJoshi commented 8 years ago

ok sure, made it to 1.6 :+1:

patric-r commented 8 years ago

Thanks so much. There's still an issue within m2eclipse which we need to accept for now: ${java.home} always resolves to the JRE in which eclipse itself is running, not to the project's JRE (which might differ). I think this also caused my issues some weeks ago.

However, it should not stop us from progressing regarding mavenizing: I'll merge your pull request now.

Thank you for your time and effort. You're very welcome submitting further patches ;)

JigarJoshi commented 8 years ago

:+1: