Bugzilla will be upgraded to version 5.0 on a still to be determined date in the near future. The original upgrade date has been delayed.
Bug 594414 - Review Request: ezmorph - Object transformation library for Java
Review Request: ezmorph - Object transformation library for Java
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
13
All Linux
low Severity medium
: ---
: ---
Assigned To: Stanislav Ochotnicky
Fedora Extras Quality Assurance
:
Depends On:
Blocks: 594416 609012
  Show dependency treegraph
 
Reported: 2010-05-20 11:44 EDT by Lubomir Rintel
Modified: 2010-07-14 13:25 EDT (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2010-07-14 13:25:37 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
sochotni: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Lubomir Rintel 2010-05-20 11:44:08 EDT
SPEC: http://v3.sk/~lkundrak/SPECS/ezmorph.spec
SRPM: http://v3.sk/~lkundrak/SRPMS/ezmorph-1.0.6-1.fc13.src.rpm

Description:

EZMorph is simple java library for transforming an Object to another
Object. It supports transformations for primitives and Objects and
multidimensional arrays.
Comment 1 Alexander Kurtakov 2010-05-20 12:41:14 EDT
Is there a reason to not build with maven as upstream is doing?
Comment 2 Lubomir Rintel 2010-05-20 13:15:23 EDT
Alexander, upstream jarball did contain neither Ant build file nor Maven POM.
Comment 3 Alexander Kurtakov 2010-05-20 13:45:17 EDT
I've been looking at http://ezmorph.cvs.sourceforge.net/viewvc/ezmorph/ezmorph/ .
Maybe CVS snapshot is more appropriate in cases like this.
Comment 4 Lubomir Rintel 2010-05-20 14:13:16 EDT
A pity and shame on Maven. It makes things so much more ugly :]
I'll adjust the package. Thank you.
Comment 5 Lubomir Rintel 2010-06-17 10:28:59 EDT
SPEC: http://v3.sk/~lkundrak/SPECS/ezmorph.spec
SRPM: http://v3.sk/~lkundrak/SRPMS/ezmorph-1.0.6-2.fc13.src.rpm

Sorry for the delay.
Comment 6 Stanislav Ochotnicky 2010-07-08 08:45:20 EDT
rpmlint was mostly OK (package is missing any LICENSE file though...not a fatal error but should be fixed with upstream).

There are a few issues though:
 * Please provide real reason for using CVS instead of release tarball. It might be funny, but once someone else starts maintaining this they will not know why it's there...
 * Why are you patching pom.xml? The package builds fine without that patch1
 * Package seems to be missing log4j as a Requires (see http://ezmorph.sourceforge.net/dependencies.html)
 * There was recent discussion on requiring javadoc subpackage to pull in main package as well (perhaps someone wants just the docs?). This is of course totally up to you, but you might consider replacing dependency on name = version with jpackage-utils.
Comment 7 Lubomir Rintel 2010-07-09 05:02:16 EDT
(In reply to comment #6)
> rpmlint was mostly OK (package is missing any LICENSE file though...not a fatal
> error but should be fixed with upstream).
> 
> There are a few issues though:
>  * Please provide real reason for using CVS instead of release tarball. It
> might be funny, but once someone else starts maintaining this they will not
> know why it's there...

Well, it exactly states what's the reason. The package could really be just rebuilt with "find -name \*.java |xargs javac", but we're choosing to use maven instead.

Nevertheless, I'll attempt to improve by commend (removing possible personal bias):

# A plain jarball with the source is provided by upstream.  We could use
# it, but we choose to build with maven for the sake of consistency.
# Therefore we pull the tree with maven metadata from VCS.

I'll not spin a new package just for a comment change now, and delay it until full review is done or more issues are found that need a change in a package.

>  * Why are you patching pom.xml? The package builds fine without that patch1

Are you sure? At the very least I would have to add more BUildRequires; I've decided to keep the build dependency chain thin.

>  * Package seems to be missing log4j as a Requires (see
> http://ezmorph.sourceforge.net/dependencies.html)

There are classes which are usable without the dependency shipped with the package. It's been my understanding that as long as you've not providing anything like an executable launcher script that would construct a class path from another packages it does not make any sense to pull in dependencies.

>  * There was recent discussion on requiring javadoc subpackage to pull in main
> package as well (perhaps someone wants just the docs?). This is of course
> totally up to you, but you might consider replacing dependency on name =
> version with jpackage-utils.    

Well, my usual motivation is pulling it the main package so I can ensure that user has the %doc file with license text installed and avoiding putting it into multiple packages. However it's not the case here, here it's mostly for the sake of consistency.

I'll prefer to keep it as it is until a guideline is made, and then I (or someone else) will eventually adjust all other packages.
Comment 8 Stanislav Ochotnicky 2010-07-13 06:13:09 EDT
OK: rpmlint must be run on every package. The output should be posted in the review.
ezmorph.noarch: W: no-documentation
ezmorph.noarch: W: non-conffile-in-etc /etc/maven/fragments/ezmorph
ezmorph.src: W: invalid-url Source0: ezmorph-1.0.6.tar.gz
3 packages and 0 specfiles checked; 0 errors, 3 warnings.

OK-SORT OF: The package must be named according to the Package Naming Guidelines.

The upstream uses name "EZMorph" everywhere. I would suggest you rename
this package so that it matches upstream.

OK: The spec file name must match the base package %{name}, in the format %{name}.spec unless your package has an exemption.  .
OK: The package must meet the Packaging Guidelines .
OK: The package must be licensed with a Fedora approved license and meet the Licensing Guidelines .
OK: The License field in the package spec file must match the actual license. 
OK: If (and only if) the source package includes the text of the license(s) in its own file, then that file, containing the text of the license(s) for the package must be included in %doc.

It would be good to contact upstream and ask them to include LICENSE file
in the CVS

OK: The spec file must be written in American English. 
OK: The spec file for the package MUST be legible. 
OK: The sources used to build the package must match the upstream source, as provided in the spec URL. Reviewers should use md5sum for this task. If no upstream URL can be specified for this package, please see the Source URL Guidelines for how to deal with this.

Since we are creating the tar file...it would actually be nice to make
it as small as possible. Please use LZMA compressionL
tar caf ezmorph-1.0.6.tar.xz --exclude CVS ezmorph-1.0.6

OK: The package MUST successfully compile and build into binary rpms on at least one primary architecture. 
OK: All build dependencies must be listed in BuildRequires, except for any that are listed in the exceptions section of the Packaging Guidelines ; inclusion of those as BuildRequires is optional. Apply common sense.
OK: Packages must NOT bundle copies of system libraries.
OK: A package must own all directories that it creates. If it does not create a directory that it uses, then it should require a package which does create that directory. 
OK: A Fedora package must not list a file more than once in the spec file's %files listings. 
OK: Permissions on files must be set properly. Executables should be set with executable permissions, for example. Every %files section must include a %defattr(...) line. 
OK: Each package must consistently use macros. 
OK: The package must contain code, or permissable content. 
OK: Large documentation files must go in a -doc subpackage. (The definition of large is left up to the packager's best judgement, but is not restricted to size. Large can refer to either size or quantity). 
OK: If a package includes something as %doc, it must not affect the runtime of the application. To summarize: If it is in %doc, the program must run properly if it is not present. 
OK: Packages must not own files or directories already owned by other packages. The rule of thumb here is that the first package to be installed should own the files or directories that other packages may rely upon. This means, for example, that no package in Fedora should ever share ownership with any of the files or directories owned by the filesystem or man package. If you feel that you have a good reason to own a file or directory that another package owns, then please present that at package review time. 
OK: All filenames in rpm packages must be valid UTF-8. 


So it's just:
 * naming (upstream seems to care about it)
 * compression
 * license (optional)

Sorry I didn't notice those things earlier (especially the name...compression is not THAT important, but while you are changing the spec...)
Comment 9 Lubomir Rintel 2010-07-13 07:17:10 EDT
(In reply to comment #8)
> OK-SORT OF: The package must be named according to the Package Naming
> Guidelines.
> 
> The upstream uses name "EZMorph" everywhere. I would suggest you rename
> this package so that it matches upstream.

I disagree with this. Upstream uses "ezmorph" (lowercased) both in POM and in distfiles. Those are usually considered key to naming the package.

> Since we are creating the tar file...it would actually be nice to make
> it as small as possible. Please use LZMA compressionL
> tar caf ezmorph-1.0.6.tar.xz --exclude CVS ezmorph-1.0.6

[lkundrak@localhost SOURCES]$ time tar caf ezmorph-1.0.6.tar.xz --exclude CVS ezmorph-1.0.6

real    0m0.159s
user    0m0.109s
sys     0m0.033s

[lkundrak@localhost SOURCES]$ time tar czf ezmorph-1.0.6.tar.gz --exclude CVS ezmorph-1.0.6

real    0m0.034s
user    0m0.010s
sys     0m0.009s

[lkundrak@localhost SOURCES]$ time tar xzf ezmorph-1.0.6.tar.gz

real    0m0.031s
user    0m0.007s
sys     0m0.012s

[lkundrak@localhost SOURCES]$ time tar xaf ezmorph-1.0.6.tar.xz

real    0m0.036s
user    0m0.005s
sys     0m0.015s

[lkundrak@localhost SOURCES]$ du -sh ezmorph-1.0.6.tar.gz ezmorph-1.0.6.tar.xz
24K     ezmorph-1.0.6.tar.gz
20K     ezmorph-1.0.6.tar.xz

This is actually quite funny. What are the benefits? A single disk block?

Seriously, nothing against XZ (I already use it for bigger projects such as VirtualBox-OSE in RPM Fusion, or Inkscape where the time savings are substantial), but I'm not using it for small projects as this unless it's portable enough and considered an estabilished standard. Please note that even in el5 it's still not part of default build root.

>  * license (optional)

Will mail upstream.

> Sorry I didn't notice those things earlier (especially the name...compression
> is not THAT important, but while you are changing the spec...)    

So, no new package this time either (until we solve the disputes above, if there still are any, or there's no more issues)
Comment 10 Stanislav Ochotnicky 2010-07-13 07:50:55 EDT
(In reply to comment #9)
> (In reply to comment #8)
> > OK-SORT OF: The package must be named according to the Package Naming
> > Guidelines.
> > 
> > The upstream uses name "EZMorph" everywhere. I would suggest you rename
> > this package so that it matches upstream.
> 
> I disagree with this. Upstream uses "ezmorph" (lowercased) both in POM and in
> distfiles. Those are usually considered key to naming the package.

Sure I have no problem with this, just wanted to be sure this was not accidental.

> > Since we are creating the tar file...it would actually be nice to make
> > it as small as possible. Please use LZMA compressionL
> > tar caf ezmorph-1.0.6.tar.xz --exclude CVS ezmorph-1.0.6
> 
> [lkundrak@localhost SOURCES]$ time tar caf ezmorph-1.0.6.tar.xz --exclude CVS
> ezmorph-1.0.6
> 
> real    0m0.159s
> user    0m0.109s
> sys     0m0.033s
> 
> [lkundrak@localhost SOURCES]$ time tar czf ezmorph-1.0.6.tar.gz --exclude CVS
> ezmorph-1.0.6
> 
> real    0m0.034s
> user    0m0.010s
> sys     0m0.009s
> 
> [lkundrak@localhost SOURCES]$ time tar xzf ezmorph-1.0.6.tar.gz
> 
> real    0m0.031s
> user    0m0.007s
> sys     0m0.012s
> 
> [lkundrak@localhost SOURCES]$ time tar xaf ezmorph-1.0.6.tar.xz
> 
> real    0m0.036s
> user    0m0.005s
> sys     0m0.015s
> 
> [lkundrak@localhost SOURCES]$ du -sh ezmorph-1.0.6.tar.gz ezmorph-1.0.6.tar.xz
> 24K     ezmorph-1.0.6.tar.gz
> 20K     ezmorph-1.0.6.tar.xz
> 
> This is actually quite funny. What are the benefits? A single disk block?
> 
> Seriously, nothing against XZ (I already use it for bigger projects such as
> VirtualBox-OSE in RPM Fusion, or Inkscape where the time savings are
> substantial), but I'm not using it for small projects as this unless it's
> portable enough and considered an estabilished standard. Please note that even
> in el5 it's still not part of default build root.

Like I said...suggestion to use XZ compression was based on the idea that you would be changing name... No problem here. I guess I've been working od Fedora-rawhide stuff too much :-)

> >  * license (optional)
> 
> Will mail upstream.
> 
> > Sorry I didn't notice those things earlier (especially the name...compression
> > is not THAT important, but while you are changing the spec...)    
> 
> So, no new package this time either (until we solve the disputes above, if
> there still are any, or there's no more issues)    

None of those things prevent me from approving this package. They were more of suggestions than requirements. The lower/upper case is usually up to maintainer (you), I just wanted to be sure you realized this.

Package is APPROVED.
Comment 11 Lubomir Rintel 2010-07-14 03:15:09 EDT
Thank you!

New Package CVS Request
=======================
Package Name: ezmorph
Short Description: Object transformation library for Java
Owners: lkundrak
Branches: F-12 F-13 EL-5 EL-6
Comment 12 Kevin Fenzi 2010-07-14 12:45:56 EDT
CVS done (by process-cvs-requests.py).
Comment 13 Lubomir Rintel 2010-07-14 13:25:37 EDT
Imported and built. Thank you.

Note You need to log in before you can comment on or make changes to this bug.