Spec URL: http://downloads.sourceforge.net/argtable/argtable2.spec
SRPM URL: http://downloads.sourceforge.net/argtable/argtable2-10-1.src.rpm
Description:
Argtable is an ANSI C library for parsing GNU style command line arguments.
It enables a program's command line syntax to be defined in the source code as
an array of argtable structs. The command line is then parsed according to that
specification and the resulting values are returned in those same structs where
they are accessible to the main program. Both tagged (-v, --verbose, --foo=bar)
and untagged arguments are supported, as are multiple instances of each
argument. Syntax error handling is automatic and the library also provides the
means for displaying the command line syntax directly from the array of
argument specifications.
Argtable can function as a "getopt_long" replacement, without the user of the
program noticing the difference. Unlike "getopt_long", argtable is
cross platform and works on Windows and Mac as well as Posix systems.

This is just an informal review - I hope it helps anyway:
build test:
- package builds fine on F10 and F11 for all architectures
major issue:
- md5sum of argtable2-10.tar.gz contained in the src.rpm doesn't match the upstream package
minor issues:
- I don't know whether there are strict rules regarding the documentation, but I would rather put the content of /usr/share/doc/argtable2 into /usr/share/doc/argtable2-devel-10, because the documentation seems to be necessary only for development purposes.
- according to http://fedoraproject.org/wiki/Packaging/SourceURL#Sourceforge.net Source0 should be http://downloads.sourceforge.net/argtable/%{name}-%{version}.tar.gz (and not prdownloads.sf.net)
- rpmlint:
rpmlint SRPMS/argtable2-10-1.fc10.src.rpm RPMS/i386/argtable2-* SPECS/argtable2.spec
argtable2-devel.i386: W: hidden-file-or-dir /usr/share/doc/argtable2-devel-10/tests/.deps
argtable2-devel.i386: W: hidden-file-or-dir /usr/share/doc/argtable2-devel-10/tests/.deps
I've had a look at this tests directory and since it was copied out of an source tree which uses auto* tools it cannot be used easily ouside. E.g. copy the directory and try a make fails:
make: *** No rule to make target `../configure.ac', needed by `Makefile.in'. Stop.
Additionally it looks like that the tests really work only when built from within the source, since they use includes like this:
fntests.c:
#include "../src/argtable2.h"
#include <assert.h>
Since the tests cannot be compiled just with the installed header files of the library anyway, probably it would be better not to package them at all.
argtable2-devel.i386: W: spurious-executable-perm /usr/share/doc/argtable2-devel-10/tests/fntests.sh
argtable2-devel.i386: W: spurious-executable-perm /usr/share/doc/argtable2-devel-10/tests/test_dbl.sh
argtable2-devel.i386: W: spurious-executable-perm /usr/share/doc/argtable2-devel-10/tests/test_int.sh
argtable2-devel.i386: W: spurious-executable-perm /usr/share/doc/argtable2-devel-10/tests/test_date.sh
argtable2-devel.i386: W: spurious-executable-perm /usr/share/doc/argtable2-devel-10/tests/test_rex.sh
argtable2-devel.i386: W: spurious-executable-perm /usr/share/doc/argtable2-devel-10/tests/test_lit.sh
4 packages and 1 specfiles checked; 0 errors, 8 warnings.
Since these are shell scripts it should be OK to that they are executable, however I suggest not to package the tests at all.
Instead of "tests" it would be an option to package "examples" into /usr/share/doc/argtable2-devel-10/: "examples" contains a bunch of good examples and can be compiled (even outside of the source tree) easily.

Hello Jess,
good:
- all mentioned issues solved
- content of the binary packages is now ok
very minor issue - just for completeness:
- omit the macro in the changelog of the spec file, otherwise rpmlint will complain:
SPECS/argtable2.spec:66: W: macro-in-%changelog _defaultdocdir
- you've added the spec file to the sources, so the sources in the provided src.rpm are slightly different from the ones on sf.net
Besides these two little minor issues I think the package looks very good.
Since I'm not an "offical" reviewer I could only help you up to this point and so somebody with that status has to do the final formal review to give you the approval for the new package.

* It is good packaging-practice to add
%check
make check
after the %install section.
* The -devel subpackage refers to "libargtable2" while the package is called "argtable2" and the main pkg description calls the software "Argtable". This inconsistency causes minor confusion. You could substitute "libargtable2" with the project name, package %{name} or even "Argtable library".
* The %doc examples/Makefile.nmake is for MSVC.
* The %doc examples/Makefile contains a hardcoded '/lib', which won't work on 64-bit platforms. [As a side-note: Overriding -Iheader/-Llibrary search-paths with default search-paths is a wide-spread mistake that causes unwanted side-effects. Here it's just examples and therefore neglectable, but where you see it in other software releases, try to get rid of it.]
* The %doc files are large enough to justify creating a -doc subpackage and moving them there. If every -devel package included so much documentation, creating buildroots and -devel spins would require a lot more resources. argtable2-devel shrinks down to 6K from 3M (!).

Hello Michael,
Thanks for the review.
I've contacted the argtable2 maintainer, I host the src.rpm and spec file in his site. He said he is working on a newer version which will also include the change to example/Makefile and will upload the revised RPMS and spec file within a few days.
For now, the spec file can be checked out here:
cvs -d:pserver:anonymous@jessrpms.cvs.sourceforge.net:/cvsroot/jessrpms login
cvs -z3 -d:pserver:anonymous@jessrpms.cvs.sourceforge.net:/cvsroot/jessrpms co -P SPECS/argtable2.spec
I applied all requested changes and added a changelog entry.
Thanks,

(In reply to comment #10)
> Hello Michael,
>
> Thanks for the review.
> I've contacted the argtable2 maintainer, I host the src.rpm and spec file in
> his site. He said he is working on a newer version which will also include the
> change to example/Makefile and will upload the revised RPMS and spec file
> within a few days.
>
> For now, the spec file can be checked out here:
> cvs -d:pserver:anonymous@jessrpms.cvs.sourceforge.net:/cvsroot/jessrpms login
> cvs -z3 -d:pserver:anonymous@jessrpms.cvs.sourceforge.net:/cvsroot/jessrpms co
> -P SPECS/argtable2.spec
Please, put the specfile some place easily available.
The SPEC from the sourceforge page doesn't even open in firefox. You can use e.g. your fedoraproject account to host the spec and srpms.

Also:
- Remove the heading space from the description
- Refer to argtable2 instead of libargtable2 in the devel package.
- You probably don't need to use --docdir in the %configure phase. After install, you should move the documentation back into the build directory, e.g.
mv %{buildroot}%{_defaultdocdir}/%{name}-devel-%{version} docdir
and include the documentation with
%doc docdir/*
Also, as the documentation is large, you should branch it to a package of its own as Michael suggested.
(Also, I'm not very fond of using macros for rm, make and so on.)

(In reply to comment #13)
> Hello,
>
> All changes were applied as suggested.
> The new src.rpm can be downloaded from:
> http://downloads.sourceforge.net/argtable/argtable2-11-2.fc11.src.rpm
What about the spec file?
I'd suggest using something else than the project home page, as the review process may take many steps before everything is in order.
> I'm actually pretty fond of using macros but if you see a good reason not to,
> I'm willing to modify the spec file accordingly.
Well, it's not an official guideline; I just think it isn't good style as nothing is gained from using the macros: KISS!
Do you still need a sponsor? I can sponsor you, if you show me you know the Fedora Packaging guidelines. Thus you need to do a couple of informal reviews (as Christian did on this package), and make at least one another package submission.

(In reply to comment #15)
> Hello,
> The spec file can be obtained from:
> http://downloads.sourceforge.net/argtable/argtable2.spec
>
> About where it is hosted, it's OK, I have the ability to upload to this SF
> project whenever needed :)
Well, I wouldn't publish anything temporary. Also you won't be able to do this for every package, so you should at least know how to use a www server to host your specs & srpms (be it fedoraproject or other).
I don't like SF since it offers the spec file as binary instead of text.
> I actually do need a sponser and would appreciate your help.
> I will start performing informal reviews this weekend.
> I also have another submission here:
> https://bugzilla.redhat.com/show_bug.cgi?id=489598
Okay. Mail me or paste here links to the reviews you have made.

I'll mail you.
About uploading to a server other than SF's, I have no objection but have no such server at my disposal. If I can get an account that enables uploading to the fedoraproject server, that would be great, am I authorized to do this with my current account?

(In reply to comment #17)
> I'll mail you.
> About uploading to a server other than SF's, I have no objection but have no
> such server at my disposal. If I can get an account that enables uploading to
> the fedoraproject server, that would be great, am I authorized to do this with
> my current account?
You should be able to do so with your FAS account once you are sponsored.
http://fedoraproject.org/wiki/Infrastructure/fedorapeople.org
For now SF is fine.

Using macros for commands, which are supposed to be located in $PATH, doesn't add any value.
For example, a "configure" script would fail, if it searched for "cp" in $PATH, but an RPM build environment redefined %__cp to something outside $PATH. Same applies to lots of other tools. Their macro values expand to absolute path, but none of these paths are passed to the configure scripts, make, or other build frameworks. Even with major redefinition of macro values, you could not fully customise the rpmbuild without modifying the spec/src.rpm.
Often, macro usage adds further inconsistencies even directly in the spec files:
> %{__rm} -rf $RPM_BUILD_ROOT> find $RPM_BUILD_ROOT -type f -name '*.la' -exec rm -f {} \;
Once %{__rm}, below plain "rm". "find" is macro-less. /sbin/ldconfig in the scriptlets is macro-less, too.

I would be willing to take a shot at packaging this if anyone's still interested. I'll need a sponsor though. I have one pending package review (bug 639518), but I still need to get around to doing some informal reviews.

Note

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