[PATCH] Build Thermostat using Maven

Roman Kennke rkennke at redhat.com
Wed Feb 8 08:24:29 UTC 2012


Hi Omair,

> On 02/07/2012 04:42 PM, Roman Kennke wrote:
> > Find attached the updated patch. This also incorporates the latest
> > changes from the hg repository.
> 
> I am not familiar with maven at all. I have been trying to go through
> the docs at maven.apache.org, but I still have lots of questions and
> things I am not clear on. Perhaps a few of them are real issues, but
> most of them are probably just me being ignorant.
> 
> Is there a way to do out of tree builds with maven? I prefer to build
> stuff out of the main source tree. With ant we sort of worked around
> this by creating a separate directory where we built stuff would be placed.

I never tried this, but I assume it is possible (probably by passing a
property -Dproject.build.dir or something).

> > # HG changeset patch
> > # Parent 7eb41d76973bcb5d1aded832174497add857457e
> > Setup project to use Maven for building.
> > 
> > diff --git a/.hgignore b/.hgignore
> > --- a/.hgignore
> > +++ b/.hgignore
> > @@ -1,7 +1,7 @@
> > -.settings/org.eclipse.jdt.ui.prefs
> > +.settings
> >  .project
> >  .classpath
> >  bin
> >  build
> > -dist
> > -
> > +dist/
> > +target
> 
> We should probably fix this up later so repository does not look clean
> (ie 'hg status' shows nothing) after building the jars.

Yeah. I think the above change did that for me.

> > diff --git a/pom.xml b/pom.xml
> > new file mode 100644
> > --- /dev/null
> > +++ b/pom.xml
> 
> > +  <modelVersion>4.0.0</modelVersion>
> > +
> > +  <groupId>com.redhat.thermostat</groupId>
> > +  <artifactId>thermostat</artifactId>
> 
> I am not sure what the maven convention is, but should 'thermostat' be
> repeated in groupId and artifactId ?

I don't think there is a convention. The reason why I did that was that
the artifacts are named like the artifact Id, e.g.
thermostat-agent-XYZ.jar. Leaving out the thermostat here would lead to
agent-XYZ.jar which seems wrong. Leaving out the thermostat of the group
also feels wrong.

> > +  <version>1.0-SNAPSHOT</version>
> 
> The current version tagged in mercurial is 0.1 Perhaps this should be
> 0.2-SNAPSHOT?

Yes.

> > +  <packaging>pom</packaging>
> > +
> > +  <name>Thermostat</name>
> > +  <url>http://maven.apache.org</url>
> 
> This should be http://icedtea.classpath.org/thermostat/, right?

Yes. I wasn't sure which URL to put there, so I left the default.

> > +  <build>
> > +    <pluginManagement>
> > +      <plugins>
> > +        <plugin>
> > +          <groupId>org.apache.maven.plugins</groupId>
> > +          <artifactId>maven-compiler-plugin</artifactId>
> > +          <version>2.3.2</version>
> > +          <configuration>
> > +            <source>1.7</source>
> > +            <target>1.7</target>
> > +          </configuration>
> > +        </plugin>
> > +        <plugin>
> > +          <groupId>org.apache.maven.plugins</groupId>
> > +          <artifactId>maven-pmd-plugin</artifactId>
> > +          <version>2.7</version>
> > +          <configuration>
> > +            <targetJdk>1.6</targetJdk>
> 
> Seems like a conflict between the target jdk as 1.6 here and 1.7 above
> in maven-compiler-plugin. Surprisingly, I can also build the whole thing
> using 1.6. What maven magic am I missing? :)

Nothing. The maven-pmd-plugin doesn't know about 1.7. But we could just
as well put 1.6 in the maven-compiler-plugin config, or even better have
a property somewhere to avoid hardcoding the same value in several
places.

> > diff --git a/agent/pom.xml b/agent/pom.xml
> > new file mode 100644
> > --- /dev/null
> > +++ b/agent/pom.xml
> 
> > +
> > +  <parent>
> > +    <groupId>com.redhat.thermostat</groupId>
> > +    <artifactId>thermostat</artifactId>
> > +    <version>1.0-SNAPSHOT</version>
> > +  </parent>
> 
> Will we need to update this version manually whenever we bump the
> version in the parent?

No. There's a 'mvn release' command that takes care of this.

> > +
> > +  <artifactId>thermostat-agent</artifactId>
> > +  <packaging>jar</packaging>
> > +
> > +  <name>Thermostat Agent</name>
> > +
> > +  <dependencies>
> > +    <dependency>
> > +      <groupId>junit</groupId>
> > +      <artifactId>junit</artifactId>
> > +      <version>4.10</version>
> 
> Is this the minimal version of junit that tests will be run against?

No. I put 4.10 there because it's the latest junit.

> What if someone has junit 4.9 (or 4.11)?

Maven will download 4.10 from the Maven repository and use that. If it's
not available then the build will fail. Maven is quite strict with
respect to versions. If you say you build with version X, it will use
version X and no other. That makes the build (almost) 100% reproducible.

> > +      <scope>test</scope>
> > +    </dependency>
> > +    <dependency>
> > +      <groupId>com.redhat.thermostat</groupId>
> > +      <artifactId>thermostat-common</artifactId>
> > +      <version>${project.version}</version>
> > +    </dependency>
> > +    <dependency>
> > +      <groupId>org.mongodb</groupId>
> > +      <artifactId>mongo-java-driver</artifactId>
> > +      <version>2.7.3</version>
> 
> Does this mean at least this version of mongo-java-driver is required to
> build thermostat?

It means that this version is used. See above. It probably makes sense
to not use the latest versions here, but whatever is in the target
Fedora version, right?

> > +    </dependency>
> > +    <dependency>
> > +      <groupId>com.sun</groupId>
> > +      <artifactId>tools</artifactId>
> > +      <version>1.7.0</version>
> > +      <scope>system</scope>
> > +      <systemPath>${java.home}/../lib/tools.jar</systemPath>
> > +    </dependency>
> > +  </dependencies>
> > +
> > +  <build>
> > +    <plugins>
> > +      <plugin>
> > +        <groupId>org.apache.maven.plugins</groupId>
> > +        <artifactId>maven-jar-plugin</artifactId>
> > +        <configuration>
> > +          <archive>
> > +            <manifest>
> > +              <addClasspath>true</addClasspath>
> > +              <mainClass>com.redhat.thermostat.agent.Main</mainClass>
> > +            </manifest>
> > +          </archive>
> > +        </configuration>
> 
> Looks like this isn't actually adding tools.jar to the Class-Path in the
> manifest file. Actually, would it be possible to leave the Class-Path out?

Yes of course. I did not know about the Fedora convention to leave out
the Class-Path. This changes some things, I need to leave out the
classpath and main class from the manifest and put the original
classpath changes back into the scripts.

> > --- a/config/agent.properties
> > +++ b/distribution/config/agent.properties
> > @@ -1,7 +1,7 @@
> >  ## Mongo storage configuration
> >  # To be used when agent is running with --local argument
> >  mongod_port=27518
> > -mongo_launch_script=@THERM_INSTALL_DIR@/scripts/localmongo.sh
> > +mongo_launch_script=scripts/localmongo.sh
> 
> This breaks the scripts. Both the agent and the client need to use
> @THERM_INSTALL_DIR@ as the directory where thermostat is "installed"
> (for developers this is just where thermostat's "distribution" jars are
> placed) so the paths can be resolved. The agent uses this to resolve the
> path to localmongo.sh script and the client uses this to resolve the
> path to the thermostat-agent script. Any ideas on how this can be handled?

Yeah no problem, we can do the replace thing that I did for some other
variables as well.

> >  # To be used when connecting on localhost without --local argument (cluster)
> >  mongos_port=27517
> >  ### Next few settings are used only by the mongo launch script
> > diff --git a/distribution/pom.xml b/distribution/pom.xml
> > new file mode 100644
> > --- /dev/null
> > +++ b/distribution/pom.xml
> 
> > +
> > +  <parent>
> > +    <groupId>com.redhat.thermostat</groupId>
> > +    <artifactId>thermostat</artifactId>
> > +    <version>1.0-SNAPSHOT</version>
> > +  </parent>
> > +
> > +  <artifactId>thermostat-distribution</artifactId>
> > +  <packaging>pom</packaging>
> > +
> > +  <name>Thermostat Distribution</name>
> > +  <url>http://maven.apache.org</url>
> > +
> 
> Can we leave out the url here and just inherit the one from the parent?

Yes. Good point.

> > +  <properties>
> > +    <project.build.sourceEncoding>UTF-8</project.build.sourceEncoding>
> > +  </properties>
> > +
> > +  <build>
> > +    <plugins>
> > +      <plugin>
> > +        <artifactId>maven-assembly-plugin</artifactId>
> > +        <version>2.3</version>
> > +        <executions>
> > +          <execution>
> > +            <id>dist-assembly</id>
> > +            <phase>package</phase>
> > +            <goals>
> > +              <goal>single</goal>
> > +            </goals>
> > +            <configuration>
> > +              <descriptors>
> > +                <descriptor>src/main/assembly/dist.xml</descriptor>
> > +              </descriptors>
> > +              <filters>
> > +                <filter>${project.build.directory}/filter.properties</filter>
> > +              </filters>
> > +            </configuration>
> > +          </execution>
> > +        </executions>
> > +      </plugin>
> 
> I have absolutely no idea what's going on here, but the description of
> the plugin at http://maven.apache.org/plugins/maven-assembly-plugin/
> leads me to believe that this is responsible for placing all the jars of
> project dependencies (such as jfreechart) in the target folder. Can we
> change that?

Yes, we can only place the project JARs there and use the system
installed JARs for the dependencies instead. I misunderstood how Fedora
forbits the classpath entry and that we need/want to use the system
installed dependency libs.

> Would it be possible to clean up (or change) the directory structure
> too? I find the path to client jar (for an example) rather bizarre:
> 
> thermostat/distribution/target/thermostat-distribution-1.0-SNAPSHOT-distribution/thermostat-distribution-1.0-SNAPSHOT/lib/thermostat-client-1.0-SNAPSHOT.jar

Hmm yes that is true. I will try to change it. Maybe a simple symlink
would do?

> > --- a/scripts/thermostat-agent
> > +++ b/distribution/scripts/thermostat-agent
> > @@ -37,13 +37,8 @@
> >  #####################################################################
> >  #
> >  # Some necessary variables.
> > -JAVA_DIR="@JAVA_DIR@"
> > -JAVA="@JAVA_HOME@/bin/java"
> > -JCOMMON_JAR="${JAVA_DIR}/jcommon.jar"
> > -MONGO_JAR="${JAVA_DIR}/mongo.jar"
> > -BSON_JAR="${JAVA_DIR}/bson.jar"
> > -TOOLS_JAR="@JAVA_HOME@/../lib/tools.jar"
> > -AGENT_CLASSPATH="${JCOMMON_JAR}:${MONGO_JAR}:${BSON_JAR}:${TOOLS_JAR}"
> > +# TODO: Consider to use $JAVA_HOME if it exists
> > +JAVA=/usr/bin/java
> >  # Find the directory where thermostat is installed.
> >  # Note this will not work if there are symlinks to resolve that
> >  # are not full paths.
> > @@ -51,7 +46,6 @@
> >  while [ -h "$SOURCE" ] ; do SOURCE="$(readlink "$SOURCE")"; done
> >  THERM_DIR=`dirname "$( cd -P "$( dirname "$SOURCE" )" && pwd )"`
> >  # Some other necessary variables.
> > -AGENT_CLASSPATH="${AGENT_CLASSPATH}:${THERM_DIR}/lib/thermostat-agent.jar:${THERM_DIR}/lib/thermostat-common.jar"
> >  AGENT_MAIN="com.redhat.thermostat.agent.Main"
> >  
> >  AGENT_ARGS="--local --properties ${THERM_DIR}/config/agent.properties"
> > @@ -62,7 +56,7 @@
> >  }
> >  
> >  function run_agent {
> > -  ${JAVA} -cp ${AGENT_CLASSPATH} ${AGENT_MAIN} ${AGENT_ARGS}
> > +  ${JAVA} -jar ${THERM_DIR}/lib/${agent.final.name} ${AGENT_ARGS}
> >  }
> >  
> 
> Since maven is not adding tools.jar to the classpath of the jar file,
> this change breaks the agent:
> Exception in thread "main" java.lang.NoClassDefFoundError:
> sun/jvmstat/monitor/MonitorException
> 	at
> com.redhat.thermostat.backend.BackendRegistry.<init>(BackendRegistry.java:67)
> 	at com.redhat.thermostat.agent.Main.main(Main.java:116)
> 

Oh yeah, well, I will revert all the changes to the scripts to use our
own classpath, so this will fix it.

> > diff --git a/scripts/thermostat-client-gui b/distribution/scripts/thermostat-client-gui
> > rename from scripts/thermostat-client-gui
> > rename to distribution/scripts/thermostat-client-gui
> > --- a/scripts/thermostat-client-gui
> > +++ b/distribution/scripts/thermostat-client-gui
> > @@ -37,14 +37,7 @@
> >  #####################################################################
> >  #
> >  # Some necessary variables.
> > -JAVA_DIR="@JAVA_DIR@"
> > -JAVA="@JAVA_HOME@/bin/java"
> > -JCOMMON_JAR="${JAVA_DIR}/jcommon.jar"
> > -JFREECHART_JAR="${JAVA_DIR}/jfreechart/jfreechart.jar"
> > -MONGO_JAR="${JAVA_DIR}/mongo.jar"
> > -BSON_JAR="${JAVA_DIR}/bson.jar"
> > -TOOLS_JAR="@JAVA_HOME@/../lib/tools.jar"
> > -CLIENT_CLASSPATH="${JCOMMON_JAR}:${JFREECHART_JAR}:${MONGO_JAR}:${BSON_JAR}:${TOOLS_JAR}"
> > +JAVA=/usr/bin/java
> 
> Does maven provide some way to set the location for JAVA when the code
> is being built?

Yes. But do we really want that? Wouldn't it make more sense to point to
JAVA_HOME at runtime (or fall back to just /usr/bin/java) ?


> > @@ -62,6 +53,6 @@
> >    echo "  thermostat-client-gui [args]"
> >  }
> >  
> > -${JAVA} -cp ${CLIENT_CLASSPATH} ${CLIENT_MAIN} ${CLIENT_ARGS}
> > +${JAVA} -jar ${THERM_DIR}/lib/${client.final.name} ${CLIENT_ARGS}
> >  exit $?
> >  
> 
> I am a little confused. How are the jars that provide the necessary
> packages located? Maven has no impact on run-time, right?

Through the Class-Path and Main-Class entries in the manifest.

> > diff --git a/distribution/src/main/assembly/filter.properties b/distribution/src/main/assembly/filter.properties
> > new file mode 100644
> > --- /dev/null
> > +++ b/distribution/src/main/assembly/filter.properties
> > @@ -0,0 +1,2 @@
> > +client.final.name=thermostat-client-${project.version}.jar
> > +agent.final.name=thermostat-agent-${project.version}.jar
> 
> Is it a good practice to require the versions in the jar names? I would
> be happier if the jar names were thermostat-client.jar and
> thermostat-agent.jar (especially given that the names of the directories
> where the jars are placed include the version numbers already).

I believe it's good practice, but we can just as well leave them out.
The reason why it's good practice is that Maven installs the artifacts
into a repository (at least into ~/.m2/repository) and it's entirely
possible to have several versions of the same library in there. For
example, if project A uses version 4.10 of junit and project B uses 4.9,
you will end up downloading both. Without version numbers this would be
a little confusing in the repository.

I will try to fix the above mentioned issues and resend the patch.

Cheers,
Roman




More information about the Thermostat mailing list