Bug 236642 - Review Request: Revisor - Revisor GUI
Review Request: Revisor - Revisor GUI
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Jason Tibbitts
Fedora Package Reviews List
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-04-16 18:05 EDT by Jonathan Steffan
Modified: 2007-11-30 17:12 EST (History)
2 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-06-02 01:13:45 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
tibbs: fedora‑review+
tibbs: fedora‑cvs+


Attachments (Terms of Use)
Updated Spec with review inspired corrections (2.76 KB, text/plain)
2007-04-18 04:27 EDT, Jef Spaleta
no flags Details

  None (edit)
Description Jonathan Steffan 2007-04-16 18:05:30 EDT
Spec URL: http://files.damaestro.us/revisor/revisor.spec
SRPM URL: http://files.damaestro.us/revisor/revisor-2.0.1-2.fc7.src.rpm
Description: Fedora "Spin" Graphical User Interface
Comment 1 Jeroen van Meeuwen 2007-04-16 18:10:56 EDT
Adding myself to CC
Comment 2 Jef Spaleta 2007-04-18 04:25:30 EDT
Okay initial review comments.  Attached you will find an updated specfile which
fixes some simple problems in the specfile and the important rpmlint error
message concerning consolehelper symlink.
Fixes in updated specfile:

- remove makeinstall macro
- add usermode and pam requires
- make symlink to consolehelper relative path instead of absolute path
- removed desktop-update-database scriptlets and associated deps
  Refer to http://fedoraproject.org/wiki/Packaging/ScriptletSnippets

One thing I can't easily fix is the SOURCE0 tag.
Please refer to http://fedoraproject.org/wiki/Packaging/SourceURL
and follow the directions for Using Revision Control to add the appropriate
comment blog which gives the necessary instructions to rebuild the source
tarball locally. The goal is to provide the steps necessary for me to rebuild
the tarball so that I can do the appropriate md5sum check for the tarball you
have included.

Please regenerate a new spec and srpm with these corrections and I should be
able to run through the full formal review.

-jef
Comment 3 Jef Spaleta 2007-04-18 04:27:37 EDT
Created attachment 152880 [details]
Updated Spec with review inspired corrections

This is an updated spec which includes corrections found during submission
review.
Comment 4 Jeroen van Meeuwen 2007-04-18 06:11:29 EDT
Thank you for taking the time to review our package.

Updated Source RPM:
http://revisor.fedoraunity.org/releases/revisor-2.0/revisor-2.0.1-3.fc7.src.rpm

Updated Spec file: http://revisor.fedoraunity.org/releases/revisor-2.0/revisor.spec
Comment 5 Jef Spaleta 2007-04-19 02:06:05 EDT
You really really really need to do something about the SOURCE tag as mentioned
in comment #2. 

Since the source tarball is being generated from a source control system (git in
this case if im reading the revisor development webpages correctly) and its not
available at an established static URL address as an identifiable 'release' then
the spec file must contain a comment blog which details the actual steps that
need to be taken to pull the appropriated tagged git entity and then repackage
it as a tarball. 

http://fedoraproject.org/wiki/Packaging/SourceURL has an example of how to do a
comment block for a public svn server. You should do something similar for your
git tree.

I cannot initiate a formal review if I can't confirm that the md5sum of the
tarball is as it should be.

-jef
Comment 6 Jeroen van Meeuwen 2007-04-19 03:59:50 EDT
I'm sorry, obviously you are right. I didn't regenerate the .spec after editting
the .spec.in.

I've updated the .spec file on the location mentioned above.
Comment 7 Jeroen van Meeuwen 2007-04-19 04:11:50 EDT
No I've not updated, for some reason it won't update.

New location is
http://revisor.fedoraunity.org/releases/revisor-2.0/revisor-2.0.1.spec

Thank you Jef, for your patience ;-)
Comment 8 Jef Spaleta 2007-04-20 02:34:58 EDT
hmm im getting a name resolution failure for revisor.fedoraunity.org at the
moment.  Is the result of a problem on my end or is the machine down currently?

-jef

Comment 9 Jonathan Steffan 2007-04-20 02:47:32 EDT
Yeah, the VM has been moved to another host due to Bob needing to use the
physical machine for another purpose (to replace a server having hardware issues
for $work) and as a side effect the DNS servers are down at the moment.
Minimally we are going to add my primary DNS server as a slave to help prevent
this in the future but we are also evaluating just moving the plone instance to
my dedicated zope server. Sorry Jef, it'll be back tomorrow I assume.
Comment 10 Jef Spaleta 2007-04-22 15:52:57 EDT
Okay, http://revisor.fedoraunity.org/releases/revisor-2.0/revisor-2.0.1.spec
still isn't resolving correctly. that URL gets redirected to index.html for the
fedoraunity site.

Please provide an updated url for both the current srpm and spec.

-jef
Comment 11 Jeroen van Meeuwen 2007-04-23 04:41:24 EDT
We're sorry for the inconvenience, backups were trashed.

Files are up now;
SPEC: http://revisor.fedoraunity.org/releases/revisor-2.0/revisor-2.0.1.spec
SRPM:
http://revisor.fedoraunity.org/releases/revisor-2.0/revisor-2.0.1-3.fc7.src.rpm
Comment 12 Jef Spaleta 2007-04-26 04:27:38 EDT
Okay the SOURCE0 tag still isn't good enough.
I need a valid url at which I can find the revisor-2.0.1.tar.gz tarball
or
I need a comment block in the specfile which tells me how to generate the
revisor-2.0.1.tar.gz using git commands.

-jef
Comment 13 Jeroen van Meeuwen 2007-04-26 05:05:57 EDT
Source0 tag is now:
Source0:
http://revisor.fedoraunity.org/releases/revisor-2.0/%{name}-%{version}.tar.gz

I'm not sure I understand what is wrong here, because that URL gives you the
proper tarball, located at

http://revisor.fedoraunity.org/releases/revisor-2.0/revisor-2.0.1.tar.gz
Comment 14 Jef Spaleta 2007-04-26 14:41:16 EDT
oops sorry, I had a stale srpm here that i was looking at.
The new SOURCE0 looks fine I should be able to start the formal review.

One thing, you should consider changing how the consolehelper symlink is created
in the release tarball for the next release, so we can avoid the hack to fix it
in the specfile.

-jef
Comment 15 Jef Spaleta 2007-04-26 14:47:53 EDT
Problem:  md5sum from SOURCE0 url and included sources do not match.  Please
doublecheck that you are including the tarball as listed from the SOURCE0 location.

Here's my local actions for md5sum test

>wget
http://revisor.fedoraunity.org/releases/revisor-2.0/revisor-2.0.1-3.fc7.src.rpm
>rpm -ivh revisor-2.0.1-3.fc7.src.rpm

>md5sum rpmbuild/SOURCES/revisor-2.0.1.tar.gz
1ae8d73266079f93e3d543631ce59704  rpmbuild/SOURCES/revisor-2.0.1.tar.gz

>spectool rpmbuild/SPECS/revisor.spec
Source0: http://revisor.fedoraunity.org/releases/revisor-2.0/revisor-2.0.1.tar.gz

>wget http://revisor.fedoraunity.org/releases/revisor-2.0/revisor-2.0.1.tar.gz
>md5sum revisor-2.0.1.tar.gz
6acf0e69808e014683607ab9628b5586  revisor-2.0.1.tar.gz


-jef
Comment 17 Jef Spaleta 2007-05-04 03:50:14 EDT
The GOOD
+ naming is good
+ specfile name matches base package name 
+ specfile written in english-ese and is legible
+ included source md5sum checks with upstream source as listed in SOURCE0 url
9b1bb4207f9c8a64609d1007420955ef  revisor-2.0.1.tar.gz
+ builds on x86 fedora-development in mock
+ no locales
+ not relocatable
+ clean section is okay
+ consistent use of macros
+ permissible code and content
+ items in doc are not runtime necessary
+ does not obviously own files from another package 
+ directory ownership of parent directories is accounted for in package deps.
+ No .la files
+ No devel subpackage
+ Need need to for shared libraries sciptlets
+ no need for scriptlets.

The BAD
- Licensed as GPL but COPYING file NOT included in docs!!

- install section has a problem
needs to include a desktop-file-install stanza to install the desktop file
correctly, and require desktop-file-utils Refer to
http://fedoraproject.org/wiki/Packaging/Guidelines#desktop

? permissions on pilgrim.py
Is pilgrim.py meant to be run stand-alone as an executable. You should either
strip the 
shell invocation at the top of the file or make it executable.


The Suggestions:
You can remove pam from the requires list, usermode requires pam. I know I know
i suggested it originally, based on just the directory ownership crap. But
taking a closer look the pam requires is redundant.

I haven't actually used this yet. Is there a simple example walkthrough on
usage? Like how to make a stupidly simple livedvd image or something, so I dont
have to think about the package selection but I can test the wizard interface.


rpmlint revisor-2.0.1-4.fc7.noarch.rpm
E: revisor non-executable-script
/usr/lib/python2.5/site-packages/revisor/pilgrim.py 0644
W: revisor non-conffile-in-etc /etc/revisor/conf.d/pungi-fc6-i386.conf
W: revisor non-conffile-in-etc /etc/revisor/conf.d/revisor-fc6-ppc.conf
W: revisor non-conffile-in-etc /etc/revisor/conf.d/pungi-f7-i386.conf
W: revisor non-conffile-in-etc /etc/revisor/conf.d/pungi-fc6-ppc.conf
W: revisor non-conffile-in-etc /etc/revisor/conf.d/pungi-fc6-x86_64.conf
W: revisor non-conffile-in-etc /etc/revisor/conf.d/revisor-fc6-x64_86.conf
W: revisor non-conffile-in-etc /etc/revisor/conf.d/revisor-fc6-i386.conf
W: revisor non-conffile-in-etc /etc/revisor/conf.d/sample-ks.cfg
W: revisor non-conffile-in-etc /etc/revisor/revisor.conf
W: revisor non-conffile-in-etc /etc/security/console.apps/revisor
W: revisor non-conffile-in-etc /etc/revisor/conf.d/revisor-f7-i386.conf
W: revisor non-conffile-in-etc /etc/pam.d/revisor

-- warnings are bogus, might want to patch the pilgrim.py to either be
executable or to strip
the intepreter from the first line.

rpmlint revisor-2.0.1-4.fc7.src.rpm
W: revisor strange-permission revisor.spec 0600
-- not important, but you might consider making it world and group readable by
default.
Comment 18 Jeroen van Meeuwen 2007-05-10 16:43:20 EDT
Spec: http://revisor.fedoraunity.org/releases/revisor-2.0/revisor-2.0.2.spec
Tarball: http://revisor.fedoraunity.org/releases/revisor-2.0/revisor-2.0.2.tar.gz
SRPM:
http://revisor.fedoraunity.org/releases/revisor-2.0/revisor-2.0.2-1.fc7.src.rpm

The only error I get with rpmlint is on the SRPM which still reports
strange-permission (I don't know how to solve that)
Comment 19 Max Spevack 2007-05-22 16:06:43 EDT
Can we PLEASE do whatever it takes to get this thing through review and into the
Fedora repo BEFORE the F7 release? (meaning, ASAP)

People are going to be looking for it.  Let's make sure they can have it.  We've
spent lots of time and energy building it and promoting the hell out of it.  It
would be a shame if people can't find and use it.

We're going to have a feature article in RH Magazine that talks about Revisor,
and we made a big deal about it in the press interviews that are ongoing and all
F7 release material.

What's the biggest hold up?  I assume between Jef, Steffan, and Jeroen, we can
get this thing done.  You three have enough combined brainpower to make Zod
himself nervous.
Comment 20 Jason Tibbitts 2007-05-22 16:20:52 EDT
All that's necessary is for Jef to verify that the issues he saw have been fixed
and give his approval.  Then the process of getting it branched, checked in and
built can happen.

I'm happy to tale a look if Jef is on vacation or something.
Comment 21 Jeroen van Meeuwen 2007-05-22 17:24:50 EDT
As I understand it Mr. Tibbitts is going to take Revisor to a nice Indian
restaurant, only 6 blocks from here, so I'm gonna stand-by online and wait for
the ping ;-)

Thank you Mr. Tibbitts!
Comment 22 Jason Tibbitts 2007-05-22 20:41:53 EDT
OK, assigning to me with visions of real, hot vindaloo (as opposed to "painful
for a Yankee, boring for a Texan" Boston-style vindaloo).

I'm going to build on top of Jef's review here, check the issues he raised and
trust him for the rest.  Just let me run this through mock....
Comment 23 Jason Tibbitts 2007-05-22 21:36:06 EDT
OK, that didn't take long.

You're right, rpmlint is now silent except for:

W: revisor strange-permission revisor.spec 0600

which I don't think is a blocker, but should be easily fixed by just "chmod 644
revisor.spec".  It shouldn't make much difference once it gets checked into CVS
anyway.

There is a copy of the license in the tarball; that needs to be included in the
final package as %doc.

As Jef pointed out above, I see no mention of desktop-file-install; you'll need
to add BR: desktop-file-utils and then something like this in %install:

desktop-file-install --vendor="fedora"              \
  --delete-original                                 \
  --dir=%{buildroot}%{_datadir}/applications        \
  %{buildroot}/%{_datadir}/applications/foo.desktop

The package installs fine on a handy rawhide system, shows up in the KDE menu
and starts without problems.  I note, though, that the menu entries do nothing
for me, and if I select "Rescue Image" as the type of media to compose, I get an
error dialog telling me "No media selected".  I'd wager that these are not
issues related to packaging and so aren't blockers, but you may know better than I.
Comment 24 Max Spevack 2007-05-22 23:18:37 EDT
Thanks Jason.  Looks like we should be able to get this thing into CVS pretty
easily at this point, which is going to be great.

All you, Jeroen!
Comment 25 Jonathan Steffan 2007-05-23 14:23:17 EDT
For a note about "W: revisor strange-permission revisor.spec 0600":

revisor.spec is generated by autmake. If there is anyone that knows how to fix
this, we are more then willing to fix it. I just have not figured out how.
Comment 26 Jonathan Steffan 2007-05-23 14:24:12 EDT
s/autmake/automake/
Comment 27 Jason Tibbitts 2007-05-23 14:51:56 EDT
rpmlint is complaining about the spec that's in the SRPM, not anything generated
by the build process, but I guess you could be building the SRPM with rpmbuild
-ts on the tarball.  To add to oddness, though, the spec in the tarball has mode
664.  

In any case, it's really not a big deal.  The other issues are much more
interesting, but should also be much easier to fix.

Comment 28 Jason Tibbitts 2007-05-24 23:02:29 EDT
I received an updates SRPM via IRC.  Here's a quick recap of the IRC discussion
for the record:

The desktop file gets installed properly, although desktop-file-install does
emit a warning:

/var/tmp/revisor-2.0.2-3.fc7-root/usr/share/applications/fedora-revisor.desktop:
warning: The 'Application' category is not defined by the desktop entry
specification.  Please use one of "AudioVideo", "Audio", "Video", "Development",
"Education", "Game", "Graphics", "Network", "Office", "Settings", "System",
"Utility" instead

I don't think this is a big deal, although it would be good to fix at some
point.  Either "Development" or "System" seem OK to me.

The COPYING file still doesn't get included in the package.  All you need to do
is change:
  %doc README NEWS AUTHORS
to
  %doc README NEWS AUTHORS COPYING
in your %files section.

That should be it.
Comment 29 Jason Tibbitts 2007-05-25 00:10:46 EDT
OK, I've received an updated package via IRC.

rpmlint is silent except for the aforementioned strange-permission warning on
the spec which isn't a blocker.

The desktop-file-install issue is fixed.

COPYING is included in the package.

So we're done.

APPROVED
Comment 30 Jonathan Steffan 2007-05-25 02:00:31 EDT
New Package CVS Request
=======================
Package Name: revisor
Short Description: Fedora "Spin" Graphical User Interface
Owners: jonathansteffan@gmail.com
Branches: F-7
InitialCC:
Comment 31 Tom "spot" Callaway 2007-05-25 16:24:36 EDT
cvs done.
Comment 32 Jason Tibbitts 2007-06-01 01:39:12 EDT
Please remember to close this bug once the package has been built, thanks.  (My
open ticket list is getting long.)
Comment 33 Jeroen van Meeuwen 2007-06-16 12:48:23 EDT
Package Change Request
======================
Package Name: revisor
Updated Fedora Owners: jonathansteffan@gmail.com,kanarip@kanarip.com
Updated Fedora CC:  jonathansteffan@gmail.com,kanarip@kanarip.com
Comment 34 Jonathan Steffan 2007-06-16 17:14:35 EDT
Yes, this should be done.
Comment 35 Jason Tibbitts 2007-06-18 12:40:40 EDT
Co-maintainer added.

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