This service will be undergoing maintenance at 00:00 UTC, 2016-09-28. It is expected to last about 1 hours

Bug 483434

Summary: Review Request: argtable2 - A library for parsing GNU style command line arguments
Product: [Fedora] Fedora Reporter: Jess Portnoy <kernel01>
Component: Package ReviewAssignee: Susi Lehtola <susi.lehtola>
Status: CLOSED DEFERRED QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: low    
Version: rawhideCC: bugs.michael, chkr, fedora-package-review, notting, susi.lehtola, theo148
Target Milestone: ---   
Target Release: ---   
Hardware: All   
OS: Linux   
URL: http://argtable.sourceforge.net/
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2009-07-22 08:35:30 EDT Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Bug Depends On:    
Bug Blocks: 201449    

Description Jess Portnoy 2009-02-01 05:00:56 EST
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.
Comment 1 Christian Krause 2009-02-16 18:31:57 EST
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.
Comment 2 Jess Portnoy 2009-02-19 06:59:29 EST
Thank you for your review, Christian.

New src.rpm is available from:
http://downloads.sourceforge.net/argtable/argtable2-10-2.src.rpm
Spec file URL is still:
http://downloads.sourceforge.net/argtable/argtable2.spec
Comment 3 Christian Krause 2009-02-19 14:59:03 EST
Thanks for the new packages. Here is a more detailed review:

Most issues are resolved besides some minor documentation issue and the static library.

GOOD:
* Rpmlint
rpmlint SRPMS/argtable2-10-2.fc10.src.rpm RPMS/i386/argtable2-10-2.fc10.i386.rpm RPMS/i386/argtable2-debuginfo-10-2.fc10.i386.rpm RPMS/i386/argtable2-devel-10-2.fc10.i386.rpm SPECS/argtable2.spec
4 packages and 1 specfiles checked; 0 errors, 0 warnings.
* Package name, spec file name and upstream package name match
* Download via spectool -r works
* Packaged tarball matches upstream (md5sum: 2ea4cd1b55ee250baa37a42b255ae426)
* License tag GPLv2+ matches the source (Although I've checked only a couple of files)
* License GPLv2+ is acceptable for Fedora
* License file packaged in %doc
* Mock build successfully (F10)
* Koji scratch build successful for all archs on F10 and F11:
https://koji.fedoraproject.org/koji/taskinfo?taskID=1140640
https://koji.fedoraproject.org/koji/taskinfo?taskID=1140678
* No build dependencies besides base system
* No locales included, so no locale handling needed
* Package contains libraries, ldconfig is called in %post and %postun
* %defattr used for all packages
* %clean section exists
* *.la files deleted
* Macros correctly used
* Header in -devel package
* *.so link in -devel package
* -devel package requires fully versioned base package
* rm -rf %{buildroot} in %install and %clean

NEED WORK:
* examples are included in both base and -devel package
* other files /usr/share/doc/argtable2 should be better packaged in
%doc of the devel package
* static libraries are shipped in devel package: please have a look at
http://fedoraproject.org/wiki/Packaging/Guidelines#StaticLibraries
and either put the static library in a -static package or remove it.
Comment 4 Jess Portnoy 2009-02-20 14:22:39 EST
Hello Christian,

I've omitted the archive file from the package.
Also, all documentation except the mandatory AUTHORS, COPYING, ChangeLog and README files now installs as part of the devel package.

New src.rpm is available from:
http://downloads.sourceforge.net/argtable/argtable2-10-3.src.rpm
Spec file URL is still:
http://downloads.sourceforge.net/argtable/argtable2.spec

Thanks again,
Comment 5 Christian Krause 2009-02-24 15:04:47 EST
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.
Comment 6 Jess Portnoy 2009-03-04 07:28:42 EST
Hello Christian,


Latest src.rpm is:
downloads.sourceforge.net/argtable/argtable2-10-4.src.rpm
Spec file URL is still: 
http://downloads.sourceforge.net/argtable/argtable2.spec 

Thanks again for reviewing,
Comment 7 Jason Tibbitts 2009-03-07 12:37:11 EST
I do not see that you have an account in the Fedora system yet.  Is this your first package for Fedora?
Comment 8 Jess Portnoy 2009-03-07 17:52:46 EST
(In reply to comment #7)
> I do not see that you have an account in the Fedora system yet.  Is this your
> first package for Fedora?  

Hello Jason,

Yes, it is.
Comment 9 Michael Schwendt 2009-03-08 08:01:40 EDT
* 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 (!).
Comment 10 Jess Portnoy 2009-03-09 11:32:57 EDT
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,
Comment 11 Susi Lehtola 2009-05-22 12:34:13 EDT
(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.
Comment 12 Susi Lehtola 2009-05-22 12:43:20 EDT
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.)
Comment 13 Jess Portnoy 2009-05-24 13:59:59 EDT
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

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.

Thanks,
Comment 14 Susi Lehtola 2009-05-24 14:47:26 EDT
(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.
Comment 15 Jess Portnoy 2009-05-25 06:26:19 EDT
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 :)

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

Which I expect to update shortly, the maintainer is about to release a new source ball. 

Thanks,
Comment 16 Susi Lehtola 2009-05-25 08:06:31 EDT
(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.
Comment 17 Jess Portnoy 2009-05-25 09:17:20 EDT
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?
Comment 18 Susi Lehtola 2009-05-25 09:30:43 EDT
(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.
Comment 19 Michael Schwendt 2009-06-08 06:50:57 EDT
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.
Comment 20 Susi Lehtola 2009-06-08 07:52:26 EDT
(In reply to comment #19)
> Using macros for commands, which are supposed to be located in $PATH, doesn't
> add any value.

Yeah, but using them is not forbidden.
Comment 21 Michael Schwendt 2009-06-08 08:00:02 EDT
Right, and I did not say they would be forbidden. I just replied to comment
13.
Comment 22 Susi Lehtola 2009-07-05 06:35:33 EDT
ping?
Comment 23 Susi Lehtola 2009-07-22 08:34:47 EDT
ping again jess
Comment 24 Theodore Lee 2011-02-12 01:04:00 EST
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.