[Bug 606720] Review Request: maven-shade-plugin - This plugin provides the capability to package the artifact in an uber-jar

[bugzilla at redhat.com]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.


https://bugzilla.redhat.com/show_bug.cgi?id=606720

--- Comment #6 from Victor G. Vasilyev <victor.vasilyev at sun.com> 2010-07-08 10:23:12 EDT ---
(In reply to comment #5)
> 1. jar files present in the tarball are used for testing. Notable exception was
> plexus-util that I replaced with symlink to our own plexus-util (fixed in this
> version) Rest of the jars are not installed they are just small test cases. 
OK

> 2. Hmmm, perhaps you misread the spec file? 
Sorry. I had wrong substitution of %{summary} that I've done in my mind.
Unfortunately, koji  was not accessible to me yesterday for unknown reason, and
I wasn't able to check my assumption.  

> 3. I actually didn't use macro commands anywhere in the spec file AFAIK. 
OK

> 4. I didn't create that dir, because maven creates it itself if it doesn't
> exist. But you're right...it's better to create it.
OK

> 5. Exactly because MAVEN_REPO_LOCAL is not used anywhere besides %build
> section, defining %global seems too much. 
> Maybe replacing with local %define in
> %build section, but still...it's a question of style and this seems like the
> most common style used in Fedora spec files.
OK
I only try to find a most common pattern for maven command for using it later
in the spec files. E.g. I see that in many spec files we should:
- define location of a local repo. Usually, it is
  %{_builddir}/%{name}-%{version}/.m2/repository
- create a directory for the local repo
- start mvn-jpp with the option -Dmaven.repo.local=<location of a local repo>

> 6. I am sorry I didn't warn in advance...this package was prepared only for
> dist-f14-maven221 tag, Currently it builds fine in rawhide (maven221 tag was
> merged recently...so maybe that's why it didn't work for you back then). 
> See my build on koji if you want (s)rpms
OK

> 7. Fixed
OK

> 8. Fixed
OK

> 9. This should not be needed, but apparently in the past there were some tools
> that had problems with unspecified Epochs. Some considered them -1 and some 0.
> Just to be on the safe side, Epoch 0 was used. This is just a precaution, but
> doesn't actually affect anything else beside upgrades from older maven.
OK

Since all issues are resolved I'll start formal review.

-- 
Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.