Bug 235191 - Review Request: postr - Flickr uploader
Summary: Review Request: postr - Flickr uploader
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Jef Spaleta
QA Contact: Fedora Package Reviews List
URL:
Whiteboard:
Depends On: 235189
Blocks:
TreeView+ depends on / blocked
 
Reported: 2007-04-04 12:45 UTC by Trond Danielsen
Modified: 2007-11-30 22:12 UTC (History)
0 users

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2007-07-16 22:38:16 UTC
Type: ---
Embargoed:
jspaleta: fedora-review+
dennis: fedora-cvs+


Attachments (Terms of Use)

Description Trond Danielsen 2007-04-04 12:45:04 UTC
Spec URL: ftp://open-gnss.org/pub/fedora/postr/postr.spec
SRPM URL: ftp://open-gnss.org/pub/fedora/postr/postr-0.5-1.fc7.src.rpm
Description: Postr is a tool for uploading pictures to Flickr. Its both a standalone application and a plugin for Nautilus.

- Builds in mock on rawhide i386 and fc6 x86_64
- Rpmlint output:
[trondd@localhost noarch]$ rpmlint postr-0.5-1.fc7.noarch.rpm 
E: postr only-non-binary-in-usr-lib
Python modules are installed in /usr/lib/nautilus, but they are not executable.

Comment 1 Trond Danielsen 2007-04-04 12:47:31 UTC
Notice that there is still no icon for this package, but the author has
requested one[1], so hopefully an icon will be available shortly. If anyone can
recommend a temporary icon, I would be grateful.

[1] http://www.burtonini.com/blog/computers/postr

Comment 2 Trond Danielsen 2007-04-04 12:49:02 UTC
Postr depends on nautilus-python. A package has been submitted, but is awaiting
review: https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=235189

Comment 3 Jef Spaleta 2007-04-29 04:28:40 UTC
Okay, now that nautilus-python is built in devel, I can move on with the review
of this.  Is the srpm and spec from the initial report still the most current?

-jef

Comment 4 Jef Spaleta 2007-04-29 04:29:25 UTC
Okay, now that nautilus-python is built in devel, I can move on with the review
of this.  Is the srpm and spec from the initial report still the most current?

I see from the website he got an icon submitted. Do you want to respin an srpm
and include the new icon as well?

-jef

Comment 5 Jef Spaleta 2007-04-29 05:03:26 UTC
Whoa!

the md5sum from the postr-0.5.tar.gz you include in your srpm doesn't match the
one pulled from  http://burtonini.com/computing/postr-0.5.tar.gz.

I did a quick diff of the contents, there is a difference, upstream did a quick
fix to setup.py which the tarball you are using doesn't include.

diff -u postr-0.5/setup.py /tmp/postr-0.5/setup.py 
--- postr-0.5/setup.py  2007-02-09 08:22:14.000000000 -0900
+++ /tmp/postr-0.5/setup.py     2007-04-03 23:08:51.000000000 -0800
@@ -3,7 +3,7 @@
 from distutils.core import setup
 
 setup(name='Postr',
-      version='0.4',
+      version='0.5',
       description='Flickr Uploader',
       author='Ross Burton',
       author_email='ross',

Please update your source tarball and do a srpm release bump. Might as well
include that upstream icon as a new source while your at it.

-jef

Comment 6 Trond Danielsen 2007-04-29 09:09:14 UTC
(In reply to comment #5)
> Whoa!
> 
> the md5sum from the postr-0.5.tar.gz you include in your srpm doesn't match the
> one pulled from  http://burtonini.com/computing/postr-0.5.tar.gz.
> 
> I did a quick diff of the contents, there is a difference, upstream did a quick
> fix to setup.py which the tarball you are using doesn't include.
> 
> diff -u postr-0.5/setup.py /tmp/postr-0.5/setup.py 
> --- postr-0.5/setup.py  2007-02-09 08:22:14.000000000 -0900
> +++ /tmp/postr-0.5/setup.py     2007-04-03 23:08:51.000000000 -0800
> @@ -3,7 +3,7 @@
>  from distutils.core import setup
>  
>  setup(name='Postr',
> -      version='0.4',
> +      version='0.5',
>        description='Flickr Uploader',
>        author='Ross Burton',
>        author_email='ross',
> 
> Please update your source tarball and do a srpm release bump. Might as well
> include that upstream icon as a new source while your at it.
> 
> -jef

Thanks for pointing out all the issues. The version number was wrong in setup.py
earlier, but I did not notice that a new tarball was released. I'll create a new
version tonight, with the icons from bzr included.

Comment 7 Trond Danielsen 2007-05-02 19:04:41 UTC
New version available at ftp://open-gnss.org/pub/fedora/postr/. I have
backported the icons from the development branch of Postr.

Comment 8 Jef Spaleta 2007-05-03 07:26:18 UTC
Following the spec directions for the bzr pull I get a mismatch in md5sums for
the postr-icons.tar.gz:
from bzr pull:
55230b0ae446905a554233b3e2411872  postr-icons.tar.gz
from included tarball in postr-0.5-2.fc7.src.rpm
6dc9ac66f7f2a120e04e44938f384045  ~/rpmbuild/SOURCES/postr-icons.tar.gz

Can you double-check and see if you get the same result

The md5sum for SOURCE0 looks good now
a66f58dbdc5c1a1befd3fa9cee629b52  ~/rpmbuild/SOURCES/postr-0.5.tar.gz

-jef


Comment 9 Trond Danielsen 2007-05-03 07:37:50 UTC
(In reply to comment #8)
> Following the spec directions for the bzr pull I get a mismatch in md5sums for
> the postr-icons.tar.gz:
> from bzr pull:
> 55230b0ae446905a554233b3e2411872  postr-icons.tar.gz
> from included tarball in postr-0.5-2.fc7.src.rpm
> 6dc9ac66f7f2a120e04e44938f384045  ~/rpmbuild/SOURCES/postr-icons.tar.gz
> 
> Can you double-check and see if you get the same result
> 
> The md5sum for SOURCE0 looks good now
> a66f58dbdc5c1a1befd3fa9cee629b52  ~/rpmbuild/SOURCES/postr-0.5.tar.gz
> 
> -jef
> 
Now this is strange, there must be some tar-magic behind the scene that I do not
understand:

[trondd@localhost SPECS]$ bzr branch -r 126
http://burtonini.com/bzr/postr/postr.dev/
Branched 126 revision(s).                                                      
[trondd@localhost SPECS]$ cd postr.dev/
[trondd@localhost postr.dev]$ tar czf postr-icons.tar.gz data setup.py
[trondd@localhost postr.dev]$ md5sum postr-icons.tar.gz 
48d0e5d3d231690ff51586ff5a0a172d  postr-icons.tar.gz
[trondd@localhost postr.dev]$ rm postr-icons.tar.gz 
[trondd@localhost postr.dev]$ tar czf postr-icons.tar.gz data setup.py
[trondd@localhost postr.dev]$ md5sum postr-icons.tar.gz 
f3c4cb76eb6e157612a2f7104cecb0ea  postr-icons.tar.gz

I just don't get it; I get a different MD5SUM every time...?!


Comment 10 Jef Spaleta 2007-05-03 08:17:46 UTC
The GOOD
+ naming is good
+ specfile name matches base package name 
+ Licensed as GPL and includes COPYING file accordingly
+ specfile written in english-ese and is legible
+ included source md5sum checks with upstream source as listed in SOURCE0 url
a66f58dbdc5c1a1befd3fa9cee629b52  postr-0.5.tar.gz
+ builds on x86 fedora-development in mock
+ no locales
+ not relocatable
+ permissions seem to be okay
+ clean section is okay
+ consistent use of macros
+ permissible code and content
+ items in doc are not runtime necessary
+ gui, desktop file installation looks good
+ does not obviously own files from another package 
+ install section looks good
+ No .la files
+ No devel subpackage
+ Need need to for shared libraries sciptlets
+ icon cache scriplet lets look good.

rpmlint binary clean:
postr-0.5-2.fc7.noarch.rpm
E: postr only-non-binary-in-usr-lib
  this is bogus, an typical of python files due to module tree location.

rpmlint srpm only minor warning:
postr-0.5-2.fc7.src.rpm
W: postr mixed-use-of-spaces-and-tabs (spaces: line 3, tab: line 46)
  Nuke the tabs in the whitespace of the desktop file install command and this
goes away.

Application appears to function for me as anticipated. Photos were uploaded,
there was much rejoicing.

The BAD
- SOURCE1 instructions do not produce tarball with matching md5sum. Not sure
what's going on here. I can confirm that repeated tar czf  gives different
md5sums. But tar cf  gives me something reproducible.
So if i decompress the included tarball I still get a md5sum on postr-icons.tar
compared to the one i create with bzr and tar cf data setup.py. Double check to
see if you can get consistent comparisons against the tar file instead of the
tar.gz.

- base package must own all directories it creates and directory ownership of
parent directories is accounted for in package deps

Problem: /usr/share/icons/hicolor/*/apps/ not owned by a required dep.
Solution: adds Requires: hicolor-icon-theme   

Problem: /usr/lib/nautilus/extensions-1.0/python is not owned by any package.
Suggested Solution: make the nautilus-python package create and own this
directory. Fix is outside the scope of this review. But if you can commit a fix
to the python-nautilus package with this fix , it will clear up the issue here.

-Requires
Do you really want to pull in python-twisted or do you want to pull in only a
subset of the twisted packages? Looking over the python code in postr it looks
like you technically only need to require python-twisted-core and
python-twisted-web, instead of the metapackage python-twisted.


So overall, pretty close to being approvable... that gzip behavior seems odd to me.

-jef

Comment 11 Trond Danielsen 2007-05-03 09:06:23 UTC
(In reply to comment #10)
> 
> rpmlint srpm only minor warning:
> postr-0.5-2.fc7.src.rpm
> W: postr mixed-use-of-spaces-and-tabs (spaces: line 3, tab: line 46)
>   Nuke the tabs in the whitespace of the desktop file install command and this
> goes away.
FIXED.

> Application appears to function for me as anticipated. Photos were uploaded,
> there was much rejoicing.
> 
> The BAD
> - SOURCE1 instructions do not produce tarball with matching md5sum. Not sure
> what's going on here. I can confirm that repeated tar czf  gives different
> md5sums. But tar cf  gives me something reproducible.
> So if i decompress the included tarball I still get a md5sum on postr-icons.tar
> compared to the one i create with bzr and tar cf data setup.py. Double check to
> see if you can get consistent comparisons against the tar file instead of the
> tar.gz.
tar cf [...] produces consistent results, so this should be FIXED now.

> 
> - base package must own all directories it creates and directory ownership of
> parent directories is accounted for in package deps
> 
> Problem: /usr/share/icons/hicolor/*/apps/ not owned by a required dep.
> Solution: adds Requires: hicolor-icon-theme   
FIXED.
 
> Problem: /usr/lib/nautilus/extensions-1.0/python is not owned by any package.
> Suggested Solution: make the nautilus-python package create and own this
> directory. Fix is outside the scope of this review. But if you can commit a fix
> to the python-nautilus package with this fix , it will clear up the issue here.

This has already been reported to bz and fixed :). See
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=238591

> -Requires
> Do you really want to pull in python-twisted or do you want to pull in only a
> subset of the twisted packages? Looking over the python code in postr it looks
> like you technically only need to require python-twisted-core and
> python-twisted-web, instead of the metapackage python-twisted.

I changed the requirements, and it works just fine. FIXED.

> 
> So overall, pretty close to being approvable... that gzip behavior seems odd
to me.

Strange, but the uncompressed tar works...

New versions at the same location as the previous ones.

Comment 12 Jef Spaleta 2007-05-04 06:19:01 UTC
Everything is good except for that blasted SOURCE1
the tar you include is not md5sum checking reliably against the bzr pull I do.
I explode the tars and did md5sums on all the files and everything looks good. 
Are we running into a problem associated with differences in file permissions
and ownership at the time of tar creation?  

I think I need to ask someone else what to do here. From the review guidance its
clear what the intent is.  I think maybe having the diffs/md5sums against the
included files from the exploded SOURCE1 and the bzr branch comeback clean
should be sufficient, but I want a second opinion.

-jef


Comment 13 Kevin Fenzi 2007-05-05 03:25:51 UTC
from a quick test here, all of bzr export/checkout/branch give you the files,
but with the timestamp of "now" on your local machine. ;( So I don't think you
will ever get them to match up correctly in a tar. 

It looks like for bzr downloads, we will need to have a guideline that requires
you to md5sum each file. 

Looks like handling timestamps is a feature request for bzr on it's roadmap: 
https://blueprints.launchpad.net/bzr/+spec/export-original-timestamps

Comment 14 Jef Spaleta 2007-05-05 03:39:29 UTC
APPROVED

Okay I'm approving this.
Please before you build (if you can figure out how to build in the new merged
world ) update the spec to include an explicit instruction in the comment block
telling people they will have to manually verify each file in the tarball since
bzr does not preserve timestamps yet.

-jef

Comment 15 Trond Danielsen 2007-05-05 08:12:51 UTC
New Package CVS Request
=======================
Package Name: postr
Short Description: Flickr uploader
Owners: trond.danielsen
Branches: FC-6
InitialCC: 

Comment 16 Dennis Gilmore 2007-05-05 15:55:52 UTC
cvs done


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