Bug 483434

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.

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

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.

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,

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.

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,

I do not see that you have an account in the Fedora system yet.  Is this your first package for Fedora?

(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.

* 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.sourceforge.net:/cvsroot/jessrpms login
cvs -z3 -d:pserver:anonymous.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.sourceforge.net:/cvsroot/jessrpms login
> cvs -z3 -d:pserver:anonymous.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.)

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,

(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.

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,

(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.

(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.

Right, and I did not say they would be forbidden. I just replied to comment
13.

ping?

ping again jess

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.