Bug 649662 - Review Request: python-tilecache - A web map tile caching system
Summary: Review Request: python-tilecache - A web map tile caching system
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
low
medium
Target Milestone: ---
Assignee: Michel Lind
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2010-11-04 08:40 UTC by viji
Modified: 2013-10-19 14:42 UTC (History)
5 users (show)

Fixed In Version: python-tilecache-2.11-5.fc13
Clone Of:
Environment:
Last Closed: 2010-11-11 19:01:35 UTC
Type: ---
Embargoed:
michel: fedora-review+
j: fedora-cvs+


Attachments (Terms of Use)
Patch the spec file to enable test suite (695 bytes, patch)
2010-11-11 10:39 UTC, Michel Lind
no flags Details | Diff

Description viji 2010-11-04 08:40:04 UTC
Spec URL: http://viji.fedorapeople.org/SPECS/python-tilecache.spec
SRPM URL: http://viji.fedorapeople.org/SRPMS/python-tilecache-2.11-1.fc13.src.rpm
Description: TileCache is a BSD licensed tile caching mechanism. The goal is to make it easy to set up a WMS or TMS frontend to any backend data services you might be interested in, using a pluggable caching and rendering mechanism.

TileCache was developed by MetaCarta Labs as a companion to OpenLayers. This TileCache client supports multiple different rendering backends (Mapserver, Mapnik etc). Each rendering backend also supports the ability to draw 'metatiles', where a large tile is rendered, and then chopped into smaller tiles using the Python Imaging library.

Comment 1 Thomas Spura 2010-11-04 09:09:20 UTC
Change title, so saying it here:
"This is my first package and need a sponsor"

-> Blocking FE-NEEDSPONSOR

Comment 2 Michel Lind 2010-11-04 15:47:35 UTC
Welcome to the world of Fedora packaging :)

A quick initial review:

This is a noarch package, so
- remove CFLAGS="$RPM_OPT_FLAGS" from %build
- you must declare BuildArch: noarch

Files in /etc should:
- be marked as either %config(noreplace) -- meaning that if the file is changed
  by the user, on upgrade/reinstall the config file is not touched but the new
  file saved with a .rpmnew suffix
- or marked %config(replace) -- meaning the opposite; old file saved as .rpmsave

- use %{_sysconfdir} rather than /etc

Also, if you're only targeting Fedora 12 or above, and RHEL 6 or above, BuildRoot is unnecessary. From Fedora 13 and above (alas, no RHEL release yet) the %clean section is also unnecessary; see 

https://fedoraproject.org/wiki/Packaging/Guidelines#.25clean

%python_sitelib declaration is also optional if you only target F-13+ and RHEL 6+:
https://fedoraproject.org/wiki/Packaging:Python

I'll do an updated review once these are fixed.

Comment 3 Shakthi Kannan 2010-11-04 17:12:52 UTC
#001 It should be Patch0 and not Patch01. Also when you apply it, it must be %patch0 -p1.

#002 Please post output of rpmlint on the built files.

#003 Please build the .src.rpm with koji on different target builds and let us know the results.

#004 In the changelog, use <viji [AT] fedoraproject DOT org> to avoid spammers using your direct e-mail address from your package .spec file.

#005 When you use cp, try to preserve permissions with "cp -p".

#006 The changelog entry should be 2.11-1.

Comment 4 viji 2010-11-05 12:32:36 UTC
@Michel Alexandre Salim

I have updated the files with the recommended changes, please do an updated review.

New Spec URL : http://viji.fedorapeople.org/SPECS/python-tilecache.spec
New SRPM URL : http://viji.fedorapeople.org/SRPMS/python-tilecache-2.11-1.fc13.src.rpm

Comment 5 viji 2010-11-05 12:38:58 UTC
@Shakthi Kannan

I have updated the files with your suggestions as well, please check.

Also,

1. Is it mandatory to use "cp -p", can I use "cp -a" if the directories are there, which is also equivalent to : -dR --preserve=all

2. Will build the same on koji and update the results ASAP.

3. Please find the output of:

$ rpmlint ../RPMS/noarch/python-tilecache-2.11-1.fc13.noarch.rpm 

python-tilecache.noarch: W: spelling-error %description -l en_US frontend -> fronted, front end, front-end
python-tilecache.noarch: W: spelling-error %description -l en_US backend -> backed, backbend, back end
python-tilecache.noarch: W: spelling-error %description -l en_US pluggable -> plug gable, plug-gable, plugged
python-tilecache.noarch: W: spelling-error %description -l en_US backends -> backbends, back ends, back-ends
python-tilecache.noarch: W: spelling-error %description -l en_US metatiles -> meta tiles, meta-tiles, metathesis
python-tilecache.noarch: E: non-executable-script /usr/lib/python2.6/site-packages/TileCache/Client.py 0644L /usr/bin/env
python-tilecache.noarch: W: spurious-executable-perm /usr/share/doc/python-tilecache-2.11/Example.py.txt
python-tilecache.noarch: E: non-executable-script /usr/lib/python2.6/site-packages/TileCache/Service.py 0644L /usr/bin/python
python-tilecache.noarch: W: no-manual-page-for-binary tilecache_seed.py
python-tilecache.noarch: W: no-manual-page-for-binary tilecache.fcgi
python-tilecache.noarch: W: no-manual-page-for-binary tilecache_http_server.py
python-tilecache.noarch: W: no-manual-page-for-binary tilecache_install_config.py
python-tilecache.noarch: W: no-manual-page-for-binary tilecache_clean.py
python-tilecache.noarch: W: no-manual-page-for-binary tilecache.cgi
1 packages and 0 specfiles checked; 2 errors, 12 warnings.

Comment 6 Shakthi Kannan 2010-11-05 15:32:38 UTC
#002 You need to run rpmlint on the .spec file, RPMS, and the .src.rpm. Please try to resolve the rpmlint errors.

#005 Doesn't matter which option you use, as long as you try to preserve timestamps, permissions for the shipped files.

#007 When you have submitted a .spec file for review, and you make changes after that, please increment the Release number, and mention the changes you made in the changelog section.

For example, you have updated it twice after release 1, and so the current release should be 2.11-3.

Comment 7 viji 2010-11-07 11:41:28 UTC
Please check the updated spec and source file, waiting for your review comments.

Spec URL: http://viji.fedorapeople.org/SPECS/python-tilecache.spec
SRPM URL: http://viji.fedorapeople.org/SRPMS/python-tilecache-2.11-3.fc13.src.rpm

#002 Please find rpmlint output, resolved all errors.

[mockbuild@dev06 SPECS]$ rpmlint python-tilecache.spec

python-tilecache.spec:59: W: macro-in-%changelog %clean
python-tilecache.spec: W: no-cleaning-of-buildroot %clean
python-tilecache.spec: W: no-buildroot-tag
python-tilecache.spec: W: no-%clean-section
0 packages and 1 specfiles checked; 0 errors, 4 warnings.

 
[mockbuild@dev06 SPECS]$ rpmlint /home/mockbuild/rpmbuild/RPMS/noarch/python-tilecache-2.11-3.fc13.noarch.rpm

python-tilecache.noarch: W: spelling-error %description -l en_US pluggable -> plug gable, plug-gable, plugged
python-tilecache.noarch: W: spurious-executable-perm /usr/share/doc/python-tilecache-2.11/Example.py.txt
python-tilecache.noarch: W: no-manual-page-for-binary tilecache_seed.py
python-tilecache.noarch: W: no-manual-page-for-binary tilecache.fcgi
python-tilecache.noarch: W: no-manual-page-for-binary tilecache_http_server.py
python-tilecache.noarch: W: no-manual-page-for-binary tilecache_install_config.py
python-tilecache.noarch: W: no-manual-page-for-binary tilecache_clean.py
python-tilecache.noarch: W: no-manual-page-for-binary tilecache.cgi
1 packages and 0 specfiles checked; 0 errors, 8 warnings.


[mockbuild@dev06 SPECS]$ rpmlint /home/mockbuild/rpmbuild/SRPMS/python-tilecache-2.11-3.fc13.src.rpm

python-tilecache.src: W: spelling-error %description -l en_US pluggable -> plug gable, plug-gable, plugged
python-tilecache.src:59: W: macro-in-%changelog %clean
python-tilecache.src: W: no-cleaning-of-buildroot %clean
python-tilecache.src: W: no-buildroot-tag
python-tilecache.src: W: no-%clean-section
1 packages and 0 specfiles checked; 0 errors, 5 warnings.

#007 changelog and release details are updated properly.

Comment 8 Shakthi Kannan 2010-11-07 12:22:23 UTC
#007 The .spec file should be readable. Kindly give a newline between the changelog entries.

Comment 9 Michel Lind 2010-11-07 12:39:27 UTC
(In reply to comment #7)
> Please check the updated spec and source file, waiting for your review
> comments.
> 
> Spec URL: http://viji.fedorapeople.org/SPECS/python-tilecache.spec
> SRPM URL:
> http://viji.fedorapeople.org/SRPMS/python-tilecache-2.11-3.fc13.src.rpm
> 
> #002 Please find rpmlint output, resolved all errors.
> 
> [mockbuild@dev06 SPECS]$ rpmlint python-tilecache.spec
> 
> python-tilecache.spec:59: W: macro-in-%changelog %clean
> python-tilecache.spec: W: no-cleaning-of-buildroot %clean
> python-tilecache.spec: W: no-buildroot-tag
> python-tilecache.spec: W: no-%clean-section
> 0 packages and 1 specfiles checked; 0 errors, 4 warnings.
> 
> 
> [mockbuild@dev06 SPECS]$ rpmlint
> /home/mockbuild/rpmbuild/RPMS/noarch/python-tilecache-2.11-3.fc13.noarch.rpm
> 
> python-tilecache.noarch: W: spelling-error %description -l en_US pluggable ->
> plug gable, plug-gable, plugged

This can be ignored, the spell checker is rather atrocious when it comes to technical terms

> python-tilecache.noarch: W: spurious-executable-perm
> /usr/share/doc/python-tilecache-2.11/Example.py.txt

Need to be fixed. A user should not be able to accidentally execute a file that's supposed to be documentation!

Looks like the file is already marked executable in the source tarball, so you can fix it in the %prep stage; just chmod -x it there

> python-tilecache.noarch: W: no-manual-page-for-binary tilecache_seed.py
> python-tilecache.noarch: W: no-manual-page-for-binary tilecache.fcgi
> python-tilecache.noarch: W: no-manual-page-for-binary tilecache_http_server.py
> python-tilecache.noarch: W: no-manual-page-for-binary
> tilecache_install_config.py
> python-tilecache.noarch: W: no-manual-page-for-binary tilecache_clean.py
> python-tilecache.noarch: W: no-manual-page-for-binary tilecache.cgi

Can be ignored (but if you want to borrow manpages from, say, Debian (where they normally add missing manpages) that's fine too.
> 1 packages and 0 specfiles checked; 0 errors, 8 warnings.
> 
> 
> [mockbuild@dev06 SPECS]$ rpmlint
> /home/mockbuild/rpmbuild/SRPMS/python-tilecache-2.11-3.fc13.src.rpm
> 
> python-tilecache.src: W: spelling-error %description -l en_US pluggable -> plug
> gable, plug-gable, plugged

Ignore

> python-tilecache.src:59: W: macro-in-%changelog %clean

A good practice is to double-comment the macros when you talk about them in
%changelog -- replace %clean with %%clean

> python-tilecache.src: W: no-cleaning-of-buildroot %clean
> python-tilecache.src: W: no-buildroot-tag
> python-tilecache.src: W: no-%clean-section


Those are fine (on newer Fedora versions)

> 1 packages and 0 specfiles checked; 0 errors, 5 warnings.
> 
> #007 changelog and release details are updated properly.

As Shakthi said, adding a newline between changelog entries would be greatly appreciated.

Which editor do you use for editing your specfiles? I use Emacs, and there are several commands that are a lifesavers:

1. M-x rpm-add-change-log-entry
   Inserts a new %%changelog entry with the correct date stamp, author name and
   email, and version info (you might have to customize name & email using
   M-x customize-group, then entering group name 'rpm-spec' 
2. M-x rpm-increase-release-tag
   increments the release tag
3. C-x h (select all) followd by M-x untabify
   converts all TABs in the specfile to spaces
   (so many specs suffer from a mix of tabs and spaces, making diff outputs
    hard to read)

Comment 10 Michel Lind 2010-11-07 12:46:34 UTC
Oh, and you'd want to own the /etc/TileCache directory too, since you created it and put files in it.

%dir %{_sysconfdir}/TileCache should do it.

You do *not* want to just simply say

 %{_sysconfdir}/TileCache

since that will recursively include the files underneath it, and you're already handling them separately (to mark them %config(noreplace)

Comment 11 viji 2010-11-08 08:41:22 UTC
Thank you so much for your valuable suggestions. I have updated the spec file, please find the URL for the same and SRPM.

Spec URL: http://viji.fedorapeople.org/SPECS/python-tilecache.spec
SRPM URL: http://viji.fedorapeople.org/SRPMS/python-tilecache-2.11-4.fc13.src.rpm

1. The following changes are being done

- Added newline between the changelog entries.
- Removed execute permission of Example.py.txt as it is coming under the documenation.
- Fixed the macro issue in changelog by adding double-comment.
- Moved to %%dir macro for sysconfigdir.

2. Regarding the man pages, I am not adding it now, but will definitely add to the coming releases. There are some man pages available for this package in debian, but it is bit outdated and not complete too. I have to edit those and write the missing ones also, which will take some time.

3. I am using vim as my editor, with ft-spec-plugin. Now giving a try on eclipse, will try emacs too (thank you for the tips, I am not that much used to emacs, using vi for a long time)

4. rpmlint outputs

[mockbuild@dev06 SPECS]$ rpmlint /home/mockbuild/rpmbuild/RPMS/noarch/python-tilecache-2.11-4.fc13.noarch.rpm

python-tilecache.noarch: W: spelling-error %description -l en_US pluggable -> plug gable, plug-gable, plugged
python-tilecache.noarch: W: no-manual-page-for-binary tilecache_seed.py
python-tilecache.noarch: W: no-manual-page-for-binary tilecache.fcgi
python-tilecache.noarch: W: no-manual-page-for-binary tilecache_http_server.py
python-tilecache.noarch: W: no-manual-page-for-binary tilecache_install_config.py
python-tilecache.noarch: W: no-manual-page-for-binary tilecache_clean.py
python-tilecache.noarch: W: no-manual-page-for-binary tilecache.cgi
1 packages and 0 specfiles checked; 0 errors, 7 warnings.


[mockbuild@dev06 SPECS]$ rpmlint /home/mockbuild/rpmbuild/SRPMS/python-tilecache-2.11-4.fc13.src.rpm

python-tilecache.src: W: spelling-error %description -l en_US pluggable -> plug gable, plug-gable, plugged
python-tilecache.src: W: no-cleaning-of-buildroot %clean
python-tilecache.src: W: no-buildroot-tag
python-tilecache.src: W: no-%clean-section
1 packages and 0 specfiles checked; 0 errors, 4 warnings.

[mockbuild@dev06 SPECS]$ rpmlint python-tilecache.spec 
python-tilecache.spec: W: no-cleaning-of-buildroot %clean
python-tilecache.spec: W: no-buildroot-tag
python-tilecache.spec: W: no-%clean-section
0 packages and 1 specfiles checked; 0 errors, 3 warnings.

Comment 13 Michel Lind 2010-11-11 10:39:03 UTC
All looks good. APPROVED -- please make a request in the Fedora Accounts System to join the packager group and I'll approve it.

A couple of small changes that you'd need to make:

(1) the file TileCache/Caches/S3.py is actually MIT-licensed, not BSD. It's noted in LICENSE.txt and I verified it in the source header.

Could you change the License field to "BSD and MIT" and put a comment above that line to that effect? e.g. "# TileCache/Caches/S3.py is MIT-licensed"

(2) The MD5 checksums don't match (see full review below). On the other hand doing a recursive diff on both downloads show that both are identical. And the file you bundle is marginally smaller (65616 vs 68705). When submitting, could you replace the file with freshly-downloaded source?

(3) from the Koji logs you supply, are you targeting EPEL-6 as well? In which case please re-add the %clean section; RHEL 6 is based on Fedora 12

(4) there is a test suite that you're not using. See attached patch. You can probably still ship the test suite in the documentation as it's a good example of how to use TileCache.

Ping me at salimma if you have any question in the future or need more packages reviewed; and please add me to the Cc: in the next several packages you maintain.

Full review:
#+TODO: TODO(t) WAIT(w@/!) FAIL(f@) | DONE(d) N/A(n)

* TODO Review [75%]
** DONE Names [2/2]
*** DONE Package name
*** DONE Spec name
** DONE Meets [[http://fedoraproject.org/wiki/Packaging/Guidelines][guidelines]]
** FAIL source files match upstream
   - bundled:  1a5772e5d71b7463742060ccc3c49f1f
   - upstream: ff0153452a9e88a8d00405fb58d689df
** TODO License [2/3]
*** DONE License is Fedora-approved
*** FAIL License field accurate
    - please note in license field that TileCache/Caches/S3.py
*** DONE License included iff packaged by upstream
** DONE rpmlint [2/2]
*** DONE on src.rpm
*** DONE on x86_64.rpm
** DONE Language & locale [3/3]
*** DONE Spec in US English
*** DONE Spec legible
*** N/A Use %find_lang to handle locale files
** DONE Build [3/3]
*** DONE Koji results
*** DONE BRs complete
*** DONE Directory ownership
** DONE Spec inspection [8/8]
*** DONE No duplicate files
*** DONE File permissions
*** DONE Filenames must be UTF-8
*** DONE no BuildRoot ([[https://fedoraproject.org/wiki/Packaging/Guidelines#BuildRoot_tag][except if targeting EPEL5]])
*** DONE Has %clean section
    (except F-13+:
    https://fedoraproject.org/wiki/Packaging/Guidelines#.25clean)
*** DONE %buildroot cleaned on %install
*** DONE Macro usage consistent
*** DONE Documentation [2/2]
**** N/A If large docs, separate -doc
**** DONE %doc files are non-essential

Comment 14 Michel Lind 2010-11-11 10:39:56 UTC
Created attachment 459716 [details]
Patch the spec file to enable test suite

Comment 15 viji 2010-11-11 11:52:52 UTC
Spec URL: http://viji.fedorapeople.org/SPECS/postgresql-pgrouting.spec
SRPM URL: http://viji.fedorapeople.org/SRPMS/python-tilecache-2.11-5.fc14.src.rpm

Updated the changes mentioned. I am not targeting for EL6 now, there are few GIS applications need to be added to the EPEL repo to make use of this package completely (soon will be adding the same, not now, once the Fedora GIS work is over)

Comment 16 viji 2010-11-11 12:01:48 UTC
Please excuse for the typo error in Spec URL. He is the correct one

http://viji.fedorapeople.org/SPECS/python-tilecache.spec

Comment 17 Michel Lind 2010-11-11 12:07:45 UTC
Looks good -- though:
- no need to mention that you're not targeting EL6 in the changelog
- the comment about license -- there was a trailing " at the end of the line
  (my mistake; I gave you the example comment surrounded by double-quotes)

No need to post an updated spec again though (or re-bump the release number); just fix it in the spec you check in.

Go ahead and request fedora-cvs:

http://fedoraproject.org/wiki/Package_SCM_admin_requests

Comment 18 viji 2010-11-11 12:27:51 UTC
New Package SCM Request
=======================
Package Name: python-tilecache
Short Description: A web map tile caching system
Owners: viji
Branches: f13 f14
InitialCC: viji salimma

Comment 19 Jason Tibbitts 2010-11-11 15:54:50 UTC
Git done (by process-git-requests).

Comment 20 Michel Lind 2010-11-11 16:37:42 UTC
viji -- once the package is built for Rawhide, you can close this as CLOSED RAWHIDE; and create an official update for the F-13 and F-14 builds at
https://admin.fedoraproject.org/updates/new/

and make the update close this bug number (649662). That way we can close the bug early, but the F-14 and F-13 builds will also get logged in Bugzilla.

Thanks!

Comment 21 Fedora Update System 2010-11-11 18:34:20 UTC
python-tilecache-2.11-5.fc14 has been submitted as an update for Fedora 14.
https://admin.fedoraproject.org/updates/python-tilecache-2.11-5.fc14

Comment 22 Fedora Update System 2010-11-11 18:34:26 UTC
python-tilecache-2.11-5.fc13 has been submitted as an update for Fedora 13.
https://admin.fedoraproject.org/updates/python-tilecache-2.11-5.fc13

Comment 23 viji 2010-11-11 19:01:13 UTC
The packages are ready for rawhide, f13 and f14.

Thank you so much for your support...

Comment 24 Fedora Update System 2010-11-22 22:11:50 UTC
python-tilecache-2.11-5.fc14 has been pushed to the Fedora 14 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 25 Fedora Update System 2010-11-22 22:25:21 UTC
python-tilecache-2.11-5.fc13 has been pushed to the Fedora 13 stable repository.  If problems still persist, please make note of it in this bug report.


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