Bug 515752

Additional note: this is my first package, and I need a sponsor

I don't believe I am able to sponsor you Jordan, but I will go through a pre-review to try to speed things up (and *hopefully* help out):

- source files match upstream. 

Unable to find upstream version, but did find 0.8.1 (http://pypi.python.org/pypi/soaplib/0.8.1)

* package meets naming and versioning guidelines.
* specfile is properly named, is cleanly written and uses macros consistently.
* summary is OK.                                                              
* description is OK
* dist tag is present.
* build root is OK.
* license field matches the actual license.
* license is open source-compatible.
* license text included in package.
- latest version is being packaged.

This does not seem to be the case, can you verify?

* BuildRequires are proper.
* %clean is present.
* package builds in mock (F10 i386)
* package installs properly.
* rpmlint is silent.
* final provides and requires seem sane:
python(abi) = 2.6
pytz
rpmlib(CompressedFileNames) <= 3.0.4-1
rpmlib(FileDigests) <= 4.6.0-1
rpmlib(PartialHardlinkSets) <= 4.0.4-1
rpmlib(PayloadFilesHavePrefix) <= 4.0-1

* owns the directories it creates.
* doesn't own any directories it shouldn't.
* no duplicates in %files.
* file permissions are appropriate.
- no generically named files

/usr/lib/python2.6/site-packages/tests
/usr/lib/python2.6/site-packages/tests/serializers
/usr/lib/python2.6/site-packages/tests/serializers/__init__.py
/usr/lib/python2.6/site-packages/tests/serializers/__init__.pyc
/usr/lib/python2.6/site-packages/tests/serializers/__init__.pyo
/usr/lib/python2.6/site-packages/tests/serializers/binary_test.py
/usr/lib/python2.6/site-packages/tests/serializers/binary_test.pyc
/usr/lib/python2.6/site-packages/tests/serializers/binary_test.pyo
/usr/lib/python2.6/site-packages/tests/serializers/clazz_test.py
/usr/lib/python2.6/site-packages/tests/serializers/clazz_test.pyc
/usr/lib/python2.6/site-packages/tests/serializers/clazz_test.pyo
/usr/lib/python2.6/site-packages/tests/serializers/primitive_test.py
/usr/lib/python2.6/site-packages/tests/serializers/primitive_test.pyc
/usr/lib/python2.6/site-packages/tests/serializers/primitive_test.pyo

* code, not content.
* documentation is small, no subpackage required.
* %docs are not necessary for the proper functioning of the package.

Updated http://deathpong.org/jomara/fedora/python-soaplib/python-soaplib-0.7.2-2.20080816svn39.fc10.src.rpm to remove included tests.

Still awaiting a sponsor

(Revoking fedora-review flag, the submitter should not
 set this flag)

Jordan, I am not a sponsor, but I can offer to do another package pre-review. Will be in touch shortly.

Here goes. This is the first of a series of review comments:

Package naming guidelines:

- Package name meets defined character set: OK
- General Naming: N/A (package is a Python module)
- Separators: N/A
- Upstream naming outside specified character set: N/A
- Conflicting package names: OK (none)
- Multiple packages with the same base name: N/A
- Spec file name: OK
- Package version: OK
- Package release: OK (package is a pre-release module, has "svn" in
%{release}, uses %{?dist})
- Minor release bumps for old branches: OK
- Case Sensitivity: OK (name is all lowercase)
- Renaming/replacing existing packages: N/A (package is new, does not replace an existing package)
- Documentation SubPackages: N/A (no sub-package necessary)
- Font Packages: N/A (package is not a font package)
- Addon Packages (General): OK (package is a Python module, uses
"python-" prefix)
- Addon Packages (python modules): OK (module is named "soaplib",
package is named "python-soaplib")

One minor comment I have is that it seems rarely used to include the SVN revision _number_ in the release field. Normally, people do with <date>svn or similar. But being able to track down the actual SVN release sounds very reasonable to me.

Packaging Guidelines:

- Naming: OK
- Version and Release: OK (see comment #7 for a remark on the release tag)
- Legal/Licensing: OK, more comments to follow.
- No inclusion of pre-built binaries or libraries: OK (package includes
no prebuilt binaries, exceptions N/A)
- Spec Legibility: OK.
- Writing a package from scratch: OK
- Modifying an existing package: N/A
- Architecture Support: N/A (package does not build architecture
specific object code)
- Filesystem Layout: OK (package follows FHS)
- Rpmlint Errors: N/A (rpmlint reports no errors or warnings)
- Changelogs: OK (spec uses recommended changelog format)
- Tags: OK (package does not use Packager, Vendor, Copyright, or PreReq;
Summary has recommended format -- for Source, another comment to follow)
- BuildRoot: OK (uses one of the recommended formats, albeit not the
top-listed one)
- Prepping BuildRoot For %install: OK
- %clean: OK
- Requires: OK. See follow-up comment about requiring python
explicitly, not just transitively via pytz
- BuildRequires: OK. See follow-up comment for discussion of superfluous build
dependency on python-devel.
- Summary and description: OK.
- Trademarks in Summary or Description: OK.
- Encoding: OK, all filenames in US-ASCII.
- Documentation: OK, package contains all documentation available upstream.
- Compiler flags: N/A
- Debuginfo packages: N/A
- Devel Packages: N/A
- Pkgconfig Files: N/A
- Requiring Base Package: N/A
- Shared Libraries: N/A

rpmlint output:

$ rpmlint SPECS/python-soaplib.spec
RPMS/noarch/python-soaplib-0.7.2-2.20080816svn39.noarch.rpm
1 packages and 1 specfiles checked; 0 errors, 0 warnings.

Neither the spec nor the built RPM report any errors or warnings. Tested with rpmlint 0.91.

And here's the big review along the Review Guidelines:

    * MUST: rpmlint must be run on every package. The output should be
posted in the review.

OK. See comment #9.


    * MUST: The package must be named according to the Package Naming
Guidelines .

See comment #7. May drop svn rev from release tag, otherwise OK.


    * MUST: The spec file name must match the base package %{name}, in
the format %{name}.spec unless your package has an exemption.

OK (Package is named "python-soaplib", spec file is named
"python-soaplib.spec")


    * MUST: The package must meet the Packaging Guidelines .

See comment #8.


    * MUST: The package must be licensed with a Fedora approved license
and meet the Licensing Guidelines .

OK. Package is licensed with LGPL2 which is Fedora approved.


    * MUST: The License field in the package spec file must match the
actual license.

OK (Package is tagged "License: LGPLv2+", contains LGPL 2.1 license text
with "any later version" clause)


    * MUST: 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 must be included in %doc.

OK (Package included file named LICENSE, file contains license, file
listed in %doc)


    * MUST: The spec file must be written in American English.

OK


    * MUST: The spec file for the package MUST be legible.

OK


    * MUST: The sources used to build the package must match the
upstream source, as provided in the spec URL. Reviewers should use
md5sum for this task. If no upstream URL can be specified for this
package, please see the Source URL Guidelines for how to deal with this.

OK with a minor suggestion for improvement. As it
appears that upstream has not released any tarballs and the preferred
method of getting the upstream module seems to be fetching from SVN,
spec should follow the guideline outlined in
http://www.fedoraproject.org/wiki/Packaging/SourceURL#Using_Revision_Control (i.e. state the svn and tar commands required to build the tarball).


    * MUST: The package MUST successfully compile and build into binary
rpms on at least one primary architecture.

OK. Tested on i386.


    * MUST: If the package does not successfully compile, build or work
on an architecture, then those architectures should be listed in the
spec in ExcludeArch. Each architecture listed in ExcludeArch MUST have a
bug filed in bugzilla, describing the reason that the package does not
compile/build/work on that architecture. The bug number MUST be placed
in a comment, next to the corresponding ExcludeArch line.

N/A. Package does not compile any object code.


    * MUST: All build dependencies must be listed in BuildRequires,
except for any that are listed in the exceptions section of the
Packaging Guidelines ; inclusion of those as BuildRequires is optional.
Apply common sense.

OK. Package lists python-devel and python-setuptools in BuildRequires.
Minor note: package does not seem to generate Python bindings, so the
Python headers are not required to be present for building the package.
The spec is however in line with the requirements in
http://fedoraproject.org/wiki/Packaging:Python in this regard. As such
the BuildRequires entry is superfluous but compliant.


    * MUST: The spec file MUST handle locales properly. This is done by
using the %find_lang macro. Using %{_datadir}/locale/* is strictly
forbidden.

N/A. Package does not use localization.


    * MUST: Every binary RPM package (or subpackage) which stores shared
library files (not just symlinks) in any of the dynamic linker's default
paths, must call ldconfig in %post and %postun.

N/A.


    * MUST: Packages must NOT bundle copies of system libraries.

OK.


    * MUST: If the package is designed to be relocatable, the packager
must state this fact in the request for review, along with the
rationalization for relocation of that specific package. Without this,
use of Prefix: /usr is considered a blocker.

N/A. Package is not relocatable.


    * MUST: A package must own all directories that it creates. If it
does not create a directory that it uses, then it should require a
package which does create that directory.

NOK. Package installs files into %{python_sitelib}, which is owned by
python, yet python is not listed in the Requires list. pytz is, and this
creates a transitive dependency on python, but this seems ugly. Package
should include python in Requires list.

The "tests" subdirectory in %{python_sitelib} is probably not meant to
be installed and should be %exclude'd.

Finally, package should list %dir %{python_sitelib}/soaplib explicitly
as suggested in
http://fedoraproject.org/wiki/Packaging:Python#System_Architecture.


    * MUST: A Fedora package must not list a file more than once in the
spec file's %files listings.

OK.


    * MUST: Permissions on files must be set properly. Executables
should be set with executable permissions, for example. Every %files
section must include a %defattr(...) line.

OK. Permissions set properly, %defattr is used as recommended
(-,root,root,-).


    * MUST: Each package must have a %clean section, which contains rm
-rf %{buildroot} (or $RPM_BUILD_ROOT).

OK. %clean section contains "rm -rf $RPM_BUILD_ROOT".


    * MUST: Each package must consistently use macros.

OK.


    * MUST: The package must contain code, or permissible content.

OK (code)


    * MUST: Large documentation files must go in a -doc subpackage. (The
definition of large is left up to the packager's best judgement, but is
not restricted to size. Large can refer to either size or quantity). [19]

N/A.


    * MUST: If a package includes something as %doc, it must not affect
the runtime of the application. To summarize: If it is in %doc, the
program must run properly if it is not present.

OK.


    * MUST: Header files must be in a -devel package.

N/A.


    * MUST: Static libraries must be in a -static package.

N/A.


    * MUST: Packages containing pkgconfig(.pc) files must 'Requires:
pkgconfig' (for directory ownership and usability).

N/A. Package does not include .pc files.


    * MUST: If a package contains library files with a suffix (e.g.
libfoo.so.1.1), then library files that end in .so (without suffix) must
go in a -devel package.

N/A.


    * MUST: In the vast majority of cases, devel packages must require
the base package using a fully versioned dependency: Requires: %{name} =
%{version}-%{release}

N/A.


    * MUST: Packages must NOT contain any .la libtool archives, these
must be removed in the spec if they are built.

OK.


    * MUST: Packages containing GUI applications must include a
%{name}.desktop file, and that file must be properly installed with
desktop-file-install in the %install section. If you feel that your
packaged GUI application does not need a .desktop file, you must put a
comment in the spec file with your explanation.

N/A. Package is not a GUI app.


    * MUST: Packages must not own files or directories already owned by
other packages. The rule of thumb here is that the first package to be
installed should own the files or directories that other packages may
rely upon. This means, for example, that no package in Fedora should
ever share ownership with any of the files or directories owned by the
filesystem or man package. If you feel that you have a good reason to
own a file or directory that another package owns, then please present
that at package review time.

OK.


    * MUST: At the beginning of %install, each package MUST run rm -rf
%{buildroot} (or $RPM_BUILD_ROOT).

OK. Package runs rm -rf $RPM_BUILD_ROOT.


    * MUST: All filenames in rpm packages must be valid UTF-8.

OK.

@LINBIT:
It is appreciated if you would write the summary of your (pre-)review
at the end.

So here is my summary of list of items I would suggest to change.

- Add "python" to Requires.

- Add comment to spec explaining how to create a tar ball from SVN, as suggested in http://www.fedoraproject.org/wiki/Packaging/SourceURL#Using_Revision_Control

- Add %dir %{python_sitelib}/soaplib to %files section as suggested in
http://fedoraproject.org/wiki/Packaging:Python#System_Architecture

- Add an %exclude in %files for %{python_sitelib}/tests

- Drop SVN rev from Release (optional)

Other than that, the package looks fine to me.

I suppose the build dependency on python-devel, while superfluous, should probably stay in so as not to confuse others and maintain compliance with http://fedoraproject.org/wiki/Packaging:Python.

Some notes:
- "Requires: python" is not needed as rpmbuild automatically
  adds "Requires: python(abi) = 2.6" to the rebuilt binary rpm.

- Adding "%dir %{python_sitelib}/soaplib" is not needed because
  %files entry "%{python_sitelib}/*" already contains this.
  Note that
------------------------------------------------------
%files
foo/
------------------------------------------------------
  (while foo is a directory) contains the directory foo/ itself
  and all files/directories/etc under foo/

- I usually recomment to include revision number in the rpm release
  tag.

Other notes:
- Now Fedora recommends to use %global instead of %define:
  https://fedoraproject.org/wiki/Packaging/Guidelines#.25global_preferred_over_.25define
- python egg file must not be excluded, and it must be created
  during build process
  https://fedoraproject.org/wiki/Packaging/Python#setuptools.2Feggs
- For creating tarball from svn, it is also recommended to include
  revision number in the tarball name.
- It is recommended that you put one line between each %changelog
  entry like:
-------------------------------------------------------
* Wed Feb  4 2009 Jordan O'Mara <jsomara> - 0.7.2-2.20080816svn39
- Added patch for manually setting wsdl url

* Tue Sep 16 2008 Jordan O'Mara <jsomara> - 0.7.2-1.20080816svn39
- Initial packaging for Fedora.
-------------------------------------------------------

I have finally found enough free time to respond to the comments, incorporate them into the project, and re-submit it for evaluation!

1. I updated soaplib to version 0.8.1. They moved webhosts and incorporated my patch upstream, so it eliminates the need for the patch

2. I did my best to incorporate all of the comments and suggestions people left. RPMlint on the spec & RPMs is clean on my machine

3. This is tested on koji against F-11, F-12, and F-13

new spec: http://jomara.fedorapeople.org/python_soaplib/python-soaplib.spec
new srpm: http://jomara.fedorapeople.org/python_soaplib/python-soaplib-0.8.1-1.fc12.src.rpm

Please let me know how this looks!

Well, first of all the tarball in your srpm and the one I could download
from the URL written in your Source0 completely differ:

--------------------------------------------------------
634880 2010-05-13 06:21 python-soaplib-0.8.1-1.fc12.src/soaplib-0.8.1.tar
276480 2009-07-14 19:29 soaplib-0.8.1.tar
--------------------------------------------------------

Would you surely you the tarball provided by the upstream and recreate
the srpm?

Note that please change the release number to avoid confusion.

I had repackaged the tarball to correct the folder structure, hence the size difference (I didn't use any compression options). I worked around this issue by using -c in the %prep section instead.

Bumped the version on the specfile. New spec & SRPM at the same links as above

new spec: http://jomara.fedorapeople.org/python_soaplib/python-soaplib.spec
new srpm:
http://jomara.fedorapeople.org/python_soaplib/python-soaplib-0.8.1-2.fc12.src.rpm

My only concern now is that koji builds for RHEL5 fail with an error message I don't quite understand. The build is here : https://koji.fedoraproject.org/koji/taskinfo?taskID=2225130 . It says to look at build.log, which is essentially empty, but root.log contains the following:

-----------------------------------------------------------------------------
DEBUG util.py:280:  Executing command: ['rpm', '-Uvh', '--nodeps', '/builddir/build/originals/python-soaplib-0.8.1-2.fc12.src.rpm']
DEBUG util.py:256:  python-soaplib              warning: user jomara does not exist - using root
DEBUG util.py:256:  ##################################################
DEBUG util.py:256:  warning: group jomara does not exist - using root
DEBUG util.py:256:  error: unpacking of archive failed on file /builddir/build/SPECS/python-soaplib.spec;4c065a58: cpio: MD5 sum mismatch
DEBUG util.py:319:  Child returncode was: 1
-----------------------------------------------------------------------------

Why would there be a RHEL5 md5 sum mismatch on the spec file? Why is it looking for my username? 

The builds for RHEL6, F11, F12, and F13 are clean.

Thanks in advance.

I've seen this happen when an SRPM is build on Fedora 12 or greater and attempted to be rebuilt on RHEL5. If I remember right they are using different hashing hence you can get a mismatch. Though, unless you are trying to do a build based off of a local srpm that shouldn't happen (at least I don't think it should).

It's not really a show-stopping issue, then?

A coworker showed me a workaround for this issue; I guess in RHEL5 the default hashing algorithm is different and you get weird failures like this.

He had me add :

%global _binary_filedigest_algorithm 1
%global _source_filedigest_algorithm 1
%global _binary_payload w9.gzdio
%global _source_payload w9.gzdio

before %prep. All systems are go

spec: http://jomara.fedorapeople.org/python_soaplib/python-soaplib.spec
srpm: http://jomara.fedorapeople.org/python_soaplib/python-soaplib-0.8.1-3.fc12.src.rpm

Note 1: Changing the hash algorithm and compression algorithm is better done on the rpmbuild commandline instead of inside the spec file.  You can also do

yum install fedora-packager

rpmbuild-md5 -bs python-soaplib.spec

(reading /usr/bin/rpmbuild-md5 will show you the commandline options to pass to rpmbuild to use the correct algorithms for EL-5)

Note 2: Unless there's a legal reason to change the upstream tarball (like stripping out patent encumbered files) we should always use the upstream tarball without modification.  in the %prep section we can modify the folder structure by running mv, cp, etc.

Thanks for the input!

Regarding note 1: we put it in the spec file to make building more obvious and intuitive. I guess I could put a comment in the specfile about how to build it, but this seems kind of like a wash

Regarding note 2: I agree about using the upstream tarball - but to the best of my knowledge i AM using it ;) Am I doing something crazy in specfile that is modifying it without my knowledge?

Apologies!  I misread Comment 16 as the modified tarball still being in the package.  I see now that it's been fixed.

About putting the defines for hash and compression algorithm in the spec file. the issue there is that that causes the package to use those algorithms no matter what the build is targeted at.  On Fedora and EPEL-6, we wouldn't want this to occur as the newer hash is more secure and the newer compression is much smaller.  The build system will do the right thing without the defines when building from the vcs (as it will construct the srpm using the correct defines for that target).

All packagers who build srpms on Fedora and target EL-5 need to be aware of this issue anyway although they may not be aware of the fix.  So a comment may be appropriate but is not strictly necessary... perhaps better documentation in the wiki would make this better although where in the wiki it would be found is an open question in my mind.

That makes sense. I just put a comment in the changelog noting the difference.

Specfile : http://jomara.fedorapeople.org/python_soaplib/python-soaplib.spec
srpm : http://jomara.fedorapeople.org/python_soaplib/python-soaplib-0.8.1-4.fc12.src.rpm

Thanks for the input

Is anything else needed to move this forward? It is still assigned to "nobody"

I'll take this:

Good:
* Package named according to the Guidelines and the spec file name matches.
* License is LGPLv2+.  Since this is your first package, I'll just note this in
  case you didn't know: this library is under any version of the LGPL that the
  user chooses since there is no version number specified anywhere in the
  source code.  At present there's only two versions of the LGPL: LGPLv2 and
  LGPLv3 so LGPLv2+ is the license tag that we use.
* LICENSE text is include
* Spec file is readable
* No locales to package
* Not a shared libraries
* Not relocatable
* Permissions set appropriately
* macros used consistently
* Package contains code, not content
* Not a GUI app
* All filenames are valid UTF-8
* Package owns all the files and directories that it creates
* build in koji but see below about unittesting

Needswork:
* There's no soaplib tarball at the specified URL.
  - pypi has a soaplib linked to arksom's account that matches:
    http://github.com/downloads/arskom/soaplib/soaplib-0.8.1.tar
  - pypi has a few soaplibs that are more recent but they're alphas and betas.
    They have tarballs not being generated out of git, though.
  So at minimum, update the Source0: url to be the arksom URL.

* The setup.py says the package requires lxml so you need to add Requires:
  python-lxml

* You should run the unittests::
  %check
  %{__python} setup.py test

  You'll need to make sure that you have all of the Requirements to run the
  package at build time as well:
  BuildRequires: pytz
  BuildRequires: python-lxml

Cosmetic:
* No need to use --optimize=1 in the install.  The rpm byte compile everything for you.

rpmlint:
python-soaplib.noarch: W: spelling-error %description -l en_US Webservices -> Web services, Web-services, Services
False positive

python-soaplib.src: W: invalid-url URL: http://wiki.github.com/jkp/soaplib HTTP Error 404: Not Found
python-soaplib.src: W: invalid-url Source0: http://github.com/downloads/jkp/soaplib/soaplib-0.8.1.tar HTTP Error 404: Not Found
These are the problem with the source url no longer existing.  See my notes on arksom for how to fix.

Do you still need to be sponsored?  If so, we should have you review some other packages or submit another package to show you know what you're doing.

Here's a couple that I'm interested in seeing get in:

Redis key-value store:
https://bugzilla.redhat.com/show_bug.cgi?id=619237

Python interface for accessing redis key-value stores:
https://bugzilla.redhat.com/show_bug.cgi?id=630339

Backport of python-2.7's ordereddict for earlier python versions
https://bugzilla.redhat.com/show_bug.cgi?id=614299
Note that the packager here is not sponsored so I'll need to work on sponsoring him too after you do the review :-)

I'm also trying to encourage zope getting in, so you could pick something in NEW state off of this list as well:
https://bugzilla.redhat.com/showdependencytree.cgi?id=633138&hide_resolved=1

I'd be available on IRC (abadger1999 on irc.freenode.net) or email for you to ask questions of during the review.  After you review the package I'd take a look and see if you missed anything before sponsoring you.  And then you'd be able to approve the package.

Note -- in the above list of other packages to review... I really meant... pick one and I'll watch how you review... you don't have to do them all before i'd sponsor you. :-)

Sponsored.

Thanks again, Toshio. Updated as per your recommendations:

Specfile : http://jomara.fedorapeople.org/python_soaplib/python-soaplib.spec
srpm :
http://jomara.fedorapeople.org/python_soaplib/python-soaplib-0.8.1-4.fc12.src.rpm

Also, I've reviewed https://bugzilla.redhat.com/show_bug.cgi?id=642995 and been sponsored by Toshio.

I forgot to mention, the latest build (which correctly includes python-lxml) won't work in RHEL5 due to a version conflict with python-lxml. EPEL doesn't have a high enough version.

Builds work for RHEL6, F12 F13 F14

You should remember to bump the release whenever you update the spec file.  Other than that things are fine.

All issues fixed.  APPROVED.

New Package SCM Request
=======================
Package Name: python-soaplib
Short Description: A simple, easily extendible soap library that provides several useful tools for creating, publishing and consuming soap web services in python.
Owners: jomara
Branches: f12 f13 f14 el6
InitialCC: jomara

Git done (by process-git-requests).

This package got into Fedora, but is not retired after about 8 years.
It's way past time to close the review request.