Red Hat Bugzilla – Bug 236642
Review Request: Revisor - Revisor GUI
Last modified: 2007-11-30 17:12:02 EST
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
Adding myself to CC
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
Please regenerate a new spec and srpm with these corrections and I should be
able to run through the full formal review.
Created attachment 152880 [details]
Updated Spec with review inspired corrections
This is an updated spec which includes corrections found during submission
Thank you for taking the time to review our package.
Updated Source RPM:
Updated Spec file: http://revisor.fedoraunity.org/releases/revisor-2.0/revisor.spec
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
I cannot initiate a formal review if I can't confirm that the md5sum of the
tarball is as it should be.
I'm sorry, obviously you are right. I didn't regenerate the .spec after editting
I've updated the .spec file on the location mentioned above.
No I've not updated, for some reason it won't update.
New location is
Thank you Jef, for your patience ;-)
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?
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.
still isn't resolving correctly. that URL gets redirected to index.html for the
Please provide an updated url for both the current srpm and spec.
We're sorry for the inconvenience, backups were trashed.
Files are up now;
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
I need a comment block in the specfile which tells me how to generate the
revisor-2.0.1.tar.gz using git commands.
Source0 tag is now:
I'm not sure I understand what is wrong here, because that URL gives you the
proper tarball, located at
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.
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
>rpm -ivh revisor-2.0.1-3.fc7.src.rpm
+ 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
+ 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.
- 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
? permissions on pilgrim.py
Is pilgrim.py meant to be run stand-alone as an executable. You should either
shell invocation at the top of the file or make it executable.
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.
E: revisor non-executable-script
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.
W: revisor strange-permission revisor.spec 0600
-- not important, but you might consider making it world and group readable by
The only error I get with rpmlint is on the SRPM which still reports
strange-permission (I don't know how to solve that)
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
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.
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!
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....
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
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" \
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.
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!
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.
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
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.
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:
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",
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
%doc README NEWS AUTHORS
%doc README NEWS AUTHORS COPYING
in your %files section.
That should be it.
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.
New Package CVS Request
Package Name: revisor
Short Description: Fedora "Spin" Graphical User Interface
Please remember to close this bug once the package has been built, thanks. (My
open ticket list is getting long.)
Package Change Request
Package Name: revisor
Updated Fedora Owners: firstname.lastname@example.org,email@example.com
Updated Fedora CC: firstname.lastname@example.org,email@example.com
Yes, this should be done.