Maven Metadata Code Review

[GoogleCodeExporter]

Purpose of code changes on this branch:
This adds maven metadata to the Guice 1.0 release.

When reviewing my code changes, please focus on:
  - The correctness of the dependencies
  - The appropriateness of the names
  - The completeness of the data

Ignore
  - The awkward directory structure.  I'll move some of the folders around for Guice 2.0 to make it 
a little cleaner.

After the review, I'll tag this and deploy Guice 1.0 to the repository.

Original issue reported on code.google.com by gk5...@gmail.com on 20 Jan 2009 at 7:49

[GoogleCodeExporter]

Original comment by limpbizkit on 18 Feb 2009 at 11:01

• Changed state: Started

[GoogleCodeExporter]

I was looking at the review of the 1.0-maven branch and had a few changes to 
make, so I've attached a 
patch…

There are two files:

 # 1.0-maven.patch — all my changes in a single patch.
 # 1.0-maven.mbox — each commit packaged as an email by git format-patch.

This patch touches more than just the POM files.

 * It also changes the build.xml in order to build sources and javadoc jars.  It's highly likely that this isn't done 
in the preferred manner, so I'd welcome feedback.
 * It makes the project entirely buildable and installable from the POMs.  I've added modules so you can build 
the whole lot from guice-parent.
  * Initially, I had intended to build the source and javadoc jars using maven, but this didn't quite work out.  
You have to build the whole shebang with maven to make the javadoc plugin 
happy. So I switched to ant.
  * I suspect that using ant is the way to go so that you end up with a jar with a single fingerprint, instead of 
two slightly different jars (one built by ant and one by maven).

My comments on the branch are:

 * I don't understand why integration-parent is needed.  What's the rational instead of just making everything 
a child of guice-parent?
  * This found an accidental bug in guice-parent's `dependencyManagement` section: you need to say 
"com.google.guice" instead of ${groupId} as the groupId changes when you are in 
the integration POMs.
 * `scm/connection` and scm/developerConnection` are missing.  However, I realised when adding them that 
they really need to be specified in each POM, as they repository doesn't follow 
the layout that maven expects 
(parent/child hierarchy).
 * In guice-parent, the compiler plugin should be in a `dependencyManagement` section.
 * It would be nice to have `relativePath` in the parent sections of each POM.
 * The spring-beans `dependencyManagement` entry in the integration-parent needs to be pulled up to 
guice-parent, as spring-beans are used by the test suite (also, the scope needs 
changing to test).
 * asm and cglib dependencies are missing from guice's POM.  They should be present and marked as optional 
I guess?
  * To be honest, it'd probably be better to not have them shaded and let the normal maven dependency 
mechanism pull them in.  But I don't feel I can make that call.  Plus, it'd 
introduce two different guice jars.  
Perhaps we should attach the jarjar version as a separate artifact?
 * Some test dependencies are missing in each POM.  I'm not sure how necessary this is.
 * The main guice pom is lacking a `<name>`.

I'd really like to see Guice 2.0 go out of the door with full support for maven 
users.  Please let me know if 
there's anything I can do to help make this happen!

Original comment by dom.happ...@gmail.com on 15 Apr 2009 at 7:59

Attachments:

• 1.0-maven.patch
• 1.0-maven.mbox

[GoogleCodeExporter]

Set review issue status to: 

Original comment by dom.happ...@gmail.com on 15 Apr 2009 at 8:01

[GoogleCodeExporter]

I've addressed the issues related to correctness, but the ones related to 
enabling a maven build have been 
ignored as they are out of scope.

Original comment by gk5...@gmail.com on 26 May 2009 at 3:05

• Changed state: Fixed

[GoogleCodeExporter]

Many thanks for your work, Greg!

The actual build using maven was just me getting a bit carried away. :)

Original comment by dom.happ...@gmail.com on 26 May 2009 at 6:16