Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
Summary: Review Request: argtable2 - A library for parsing GNU style command line arguments
https://bugzilla.redhat.com/show_bug.cgi?id=483434
Summary: Review Request: argtable2 - A library for parsing GNU style command line arguments Product: Fedora Version: rawhide Platform: All OS/Version: Linux Status: NEW Severity: medium Priority: low Component: Package Review AssignedTo: nobody@fedoraproject.org ReportedBy: kernel01@gmail.com QAContact: extras-qa@fedoraproject.org CC: notting@redhat.com, fedora-package-review@redhat.com Estimated Hours: 0.0 Classification: Fedora
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.
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=483434
Jess Portnoy kernel01@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- URL| |http://argtable.sourceforge | |.net/
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=483434
Christian Krause chkr@plauener.de changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |chkr@plauener.de
--- Comment #1 from Christian Krause chkr@plauener.de 2009-02-16 18:31:57 EDT --- 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/%%7Bname%7D-%%7Bversion%7D.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.
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=483434
--- Comment #2 from Jess Portnoy kernel01@gmail.com 2009-02-19 06:59:29 EDT --- 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
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=483434
--- Comment #3 from Christian Krause chkr@plauener.de 2009-02-19 14:59:03 EDT --- 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.
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=483434
--- Comment #4 from Jess Portnoy kernel01@gmail.com 2009-02-20 14:22:39 EDT --- 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,
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=483434
--- Comment #5 from Christian Krause chkr@plauener.de 2009-02-24 15:04:47 EDT --- 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.
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=483434
--- Comment #6 from Jess Portnoy kernel01@gmail.com 2009-03-04 07:28:42 EDT --- 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,
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=483434
--- Comment #7 from Jason Tibbitts tibbs@math.uh.edu 2009-03-07 12:37:11 EDT --- I do not see that you have an account in the Fedora system yet. Is this your first package for Fedora?
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=483434
--- Comment #8 from Jess Portnoy kernel01@gmail.com 2009-03-07 17:52:46 EDT --- (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.
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=483434
Jason Tibbitts tibbs@math.uh.edu changed:
What |Removed |Added ---------------------------------------------------------------------------- Blocks| |177841(FE-NEEDSPONSOR)
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=483434
Michael Schwendt bugs.michael@gmx.net changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |bugs.michael@gmx.net
--- Comment #9 from Michael Schwendt bugs.michael@gmx.net 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 (!).
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=483434
--- Comment #10 from Jess Portnoy kernel01@gmail.com 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,
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=483434
Jussi Lehtola jussi.lehtola@iki.fi changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |jussi.lehtola@iki.fi
--- Comment #11 from Jussi Lehtola jussi.lehtola@iki.fi 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.
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=483434
--- Comment #12 from Jussi Lehtola jussi.lehtola@iki.fi 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.)
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=483434
--- Comment #13 from Jess Portnoy kernel01@gmail.com 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,
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=483434
--- Comment #14 from Jussi Lehtola jussi.lehtola@iki.fi 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.
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=483434
--- Comment #15 from Jess Portnoy kernel01@gmail.com 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,
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=483434
Jussi Lehtola jussi.lehtola@iki.fi changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED AssignedTo|nobody@fedoraproject.org |jussi.lehtola@iki.fi Flag| |fedora-review?
--- Comment #16 from Jussi Lehtola jussi.lehtola@iki.fi 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.
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=483434
Jussi Lehtola jussi.lehtola@iki.fi changed:
What |Removed |Added ---------------------------------------------------------------------------- Blocks|177841(FE-NEEDSPONSOR) |
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=483434
--- Comment #17 from Jess Portnoy kernel01@gmail.com 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?
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=483434
--- Comment #18 from Jussi Lehtola jussi.lehtola@iki.fi 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.
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=483434
--- Comment #19 from Michael Schwendt mschwendt@gmail.com 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.
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=483434
--- Comment #20 from Jussi Lehtola jussi.lehtola@iki.fi 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.
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=483434
--- Comment #21 from Michael Schwendt mschwendt@gmail.com 2009-06-08 08:00:02 EDT --- Right, and I did not say they would be forbidden. I just replied to comment 13.
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=483434
--- Comment #22 from Jussi Lehtola jussi.lehtola@iki.fi 2009-07-05 06:35:33 EDT --- ping?
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=483434
--- Comment #23 from Jussi Lehtola jussi.lehtola@iki.fi 2009-07-22 08:34:47 EDT --- ping again jess
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=483434
Jussi Lehtola jussi.lehtola@iki.fi changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |CLOSED Resolution| |DEFERRED
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=483434
Jason Tibbitts tibbs@math.uh.edu changed:
What |Removed |Added ---------------------------------------------------------------------------- Blocks| |201449(FE-DEADREVIEW) Flag|fedora-review? |
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=483434
Theodore Lee theo148@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |theo148@gmail.com
--- Comment #24 from Theodore Lee theo148@gmail.com 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.
package-review@lists.fedoraproject.org