Bug 706984 - Review Request: args4j - Small Java lib that makes it easy to parse command line options/args in CUI apps
Summary: Review Request: args4j - Small Java lib that makes it easy to parse command l...
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Marek Goldmann
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2011-05-23 16:59 UTC by Jaromír Cápík
Modified: 2016-02-01 01:54 UTC (History)
5 users (show)

Fixed In Version:
Clone Of:
Environment:
Last Closed: 2011-06-08 16:21:10 UTC
Type: ---
Embargoed:
mgoldman: fedora-review+
j: fedora-cvs+


Attachments (Terms of Use)

Description Jaromír Cápík 2011-05-23 16:59:05 UTC
Spec URL: http://jcapik.fedorapeople.org/files/args4j-site/1/args4j-site.spec
SRPM URL: http://jcapik.fedorapeople.org/files/args4j-site/1/args4j-site-2.0.16-1.fc16.src.rpm
Description: 
This package contains the parent pom file for args4j projects.

Comment 1 Alexander Kurtakov 2011-05-23 17:12:03 UTC
Why just the pom but not the whole project?
Looks like all the sources are in the same tag that has been used for fetching the pom.

Comment 2 Alexander Kurtakov 2011-05-24 06:09:34 UTC
svn export https://svn.java.net/svn/args4j~svn/tags/args4j-site-2_0_16

removing wagon-svn extension from the main pom.xml
fix ant group to be org.apache.ant

everything builds fine

Comment 3 Alexander Kurtakov 2011-05-24 06:10:22 UTC
I would really prefer to see one package for the whole args4j.

Comment 5 Jaromír Cápík 2011-05-25 09:16:43 UTC
I'm not sure about the ant runtime dependency for args4j-tools. Source files do not seem to use it.

Comment 6 Jaromír Cápík 2011-05-25 11:46:55 UTC
ant dependency removed from the args4j-tools pom.

Spec URL: http://jcapik.fedorapeople.org/files/args4j/2/args4j.spec
SRPM URL: http://jcapik.fedorapeople.org/files/args4j/2/args4j-2.0.16-2.fc14.src.rpm

Comment 7 Marek Goldmann 2011-05-26 16:00:14 UTC
I'll take this one.

Comment 8 Marek Goldmann 2011-05-26 16:38:55 UTC
Just a few small comments:

Key:
- = N/A
x = Check
! = Problem
? = Not evaluated

=== REQUIRED ITEMS ===
[x]  Rpmlint output:

$ rpmlint args4j-2.0.16-2.fc14.src.rpm
args4j.src: I: enchant-dictionary-not-found en_US
args4j.src: W: invalid-url Source0: args4j-2.0.16.tar.xz
1 packages and 0 specfiles checked; 0 errors, 1 warnings.

[x]  Package is named according to the Package Naming Guidelines[1].
[x]  Spec file name must match the base package name, in the format %{name}.spec.
[x]  Package meets the Packaging Guidelines[2].
[x]  Package successfully compiles and builds into binary rpms.
[x]  Buildroot definition is not present
[x]  Package is licensed with an open-source compatible license and meets other legal requirements as defined in the legal section of Packaging Guidelines[3,4].
[!]  License field in the package spec file matches the actual license.

Which part of the lib is licensed under BSD? Homepage states only MIT.

[x]  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 is included in %doc.
[x]  All independent sub-packages have license of their own
[x]  Spec file is legible and written in American English.
[!]  Sources used to build the package matches the upstream source, as provided in the spec URL.
MD5SUM this package    :
MD5SUM upstream package:

It seems the instructions to get the source code from SVN are not working for me. Could you please check it?

[x]  All build dependencies are listed in BuildRequires, except for any that are listed in the exceptions section of Packaging Guidelines[5].
[x]  Package must own all directories that it creates.
[-]  Package requires other packages for directories it uses.
[x]  Package does not contain duplicates in %files.

Contains license in subpackage, but this is OK.

[x]  Permissions on files are set properly.
[x]  Package does NOT have a %clean section which contains rm -rf %{buildroot} (or $RPM_BUILD_ROOT). (not needed anymore)
[x]  Package consistently uses macros (no %{buildroot} and $RPM_BUILD_ROOT mixing)
[x]  Package contains code, or permissable content.
[x]  Fully versioned dependency in subpackages, if present.
[-]  Package contains a properly installed %{name}.desktop file if it is a GUI application.
[x]  Package does not own files or directories owned by other packages.
[x]  Javadoc documentation files are generated and included in -javadoc subpackage
[x]  Javadocs are placed in %{_javadocdir}/%{name} (no -%{version} symlinks)
[x]  Packages have proper BuildRequires/Requires on jpackage-utils
[x]  Javadoc subpackages have Require: jpackage-utils
[x]  Package uses %global not %define
[!]  If package uses tarball from VCS include comment how to re-create that tarball (svn export URL, git clone URL, ...)

Instructions doesn't work.

[!]  If source tarball includes bundled jar/class files these need to be removed prior to building

Please add rm -rf args4j/lib

[x]  All filenames in rpm packages must be valid UTF-8.
[x]  Jar files are installed to %{_javadir}/%{name}.jar (see [6] for details)
[x]  If package contains pom.xml files install it (including depmaps) even when building with ant
[x]  pom files has correct add_to_maven_depmap call which resolves to the pom file (use "JPP." and "JPP-" correctly)

=== Maven ===
[x]  Use %{_mavenpomdir} macro for placing pom files instead of %{_datadir}/maven2/poms
[-]  If package uses "-Dmaven.test.skip=true" explain why it was needed in a comment
[-]  If package uses custom depmap "-Dmaven2.jpp.depmap.file=*" explain why it's needed in a comment
[x]  Package uses %update_maven_depmap in %post/%postun
[x]  Packages have Requires(post) and Requires(postun) on jpackage-utils (for %update_maven_depmap macro)

=== Other suggestions ===
[x]  If possible use upstream build method (maven/ant/javac)
[x]  Avoid having BuildRequires on exact NVR unless necessary
[x]  Package has BuildArch: noarch (if possible)
[!]  Latest version is packaged.

I don't know :) The download section on homepage gives 404. Any hints?

[x]  Reviewer should test that the package builds in mock.

http://koji.fedoraproject.org/koji/taskinfo?taskID=3093982

Comment 9 Jaromír Cápík 2011-05-30 13:22:31 UTC
Hi Marek.

I somehow haven't noticed You've done the review :]

>>Which part of the lib is licensed under BSD? Homepage states only MIT.

LICENSE.txt contained in the source tarball is BSD and the project homepage states MIT. I'll try to reach upstream again, but as I got no answer for my first question, I don't suppose I'll be more successful in case of license clarification -> don't know if we should wait for the clarification.

>>It seems the instructions to get the source code from SVN are not working for
me. Could you please check it?

You have to register on java.net and upload Your personal ssh key to the server in order to be able to export the sources. It seems that it would be better to make a note in the spec file stating this fact.

>>I don't know :) The download section on homepage gives 404. Any hints?

Yes. They don't care about the link anymore. The only way how to obtain the source is via the svn export.

Comment 10 Jaromír Cápík 2011-05-30 13:38:30 UTC
bundled lib dir removed + note about the ssh key added:

Spec URL: http://jcapik.fedorapeople.org/files/args4j/3/args4j.spec
SRPM URL: http://jcapik.fedorapeople.org/files/args4j/3/args4j-2.0.16-3.fc14.src.rpm

Comment 11 Marek Goldmann 2011-05-30 15:38:27 UTC
[x]  License field in the package spec file matches the actual license.

OK, thanks for clarification, specify both licenses then.

[!]  Sources used to build the package matches the upstream source, as provided in the spec URL.
MD5SUM this package    : b765c9c25789884cb982e7c8fefc0de0
MD5SUM upstream package: b29f50e3dd5933fb7fe498a38f9fb191

It seems they way you create the package changes the timestamps. Use xz compression method - change the instructions (and update the file in srpm) to:

# tar cafJ args4j-2.0.16.tar.xz args4j-2.0.16

[x]  If source tarball includes bundled jar/class files these need to be
removed prior to building

Thanks!

[x]  Latest version is packaged.

================
*** APPROVED ***
================

Comment 12 Jaromír Cápík 2011-05-31 09:33:13 UTC
Hi Marek.

The differences are caused by the svn export, since it produces different timestamps of the directories. That's why MD5SUM cannot be used in this case.
Only the directory diff is helpful.

BR, J.

Comment 13 Jaromír Cápík 2011-05-31 14:24:15 UTC
New Package SCM Request
=======================
Package Name: args4j
Short Description: Small Java lib that makes it easy to parse command line options/args in CUI apps
Owners: jcapik
Branches: f15
InitialCC: java-sig

Comment 14 Jason Tibbitts 2011-05-31 19:15:54 UTC
Git done (by process-git-requests).

Comment 15 Jaromír Cápík 2011-06-08 16:21:10 UTC
The package has been built successfuly:
https://koji.fedoraproject.org/koji/buildinfo?buildID=246243

Thanks for the review and git repo.

Closing.


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