Bug 212003 - Review Request: mugshot - Companion software for mugshot.org
Review Request: mugshot - Companion software for mugshot.org
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
All Linux
medium Severity medium
: ---
: ---
Assigned To: Anthony Green
Fedora Package Reviews List
Depends On:
  Show dependency treegraph
Reported: 2006-10-24 12:10 EDT by Owen Taylor
Modified: 2008-01-21 13:54 EST (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Last Closed: 2008-01-21 13:54:22 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
green: fedora‑review+
kevin: fedora‑cvs+

Attachments (Terms of Use)
Mugshot Spec file (5.61 KB, text/plain)
2006-10-24 12:17 EDT, Owen Taylor
no flags Details
Spec file with improved GConf scripts (6.84 KB, text/plain)
2006-11-06 14:04 EST, Owen Taylor
no flags Details
New version of the spec file (8.49 KB, text/plain)
2007-03-15 19:12 EDT, Owen Taylor
no flags Details
Tweaked spec file as described (9.08 KB, text/plain)
2007-03-19 15:08 EDT, Owen Taylor
no flags Details

  None (edit)
Description Owen Taylor 2006-10-24 12:10:12 EDT
Spec URL: 
I'll attach the spec file to the bug report

SRPM URL: http://devel.mugshot.org/download/sources/fedora-core-6/mugshot-1.1.20-1.fc6.src.rpm

[ I'll be a bit more verbose here than in the package's Description field ]

Mugshot is a Red Hat project to (quoting http://mugshot.org/about)
"provide a live social experience around entertainment." The feature
we are currently focusing on is summarizing what you are doing and
what your friends are doing on different sites, and providing you
with live notification of your friends activities.

Mugshot consists of a central set of servers that provide a web
interface, and client code that talks to the servers via XMPP and
enhances the user experience with live notification and chat. We 
currently provide clients for both X-based desktops and for Windows.

All the Mugshot code (both client and server side) is released under
the terms of the GNU GPL and we try to run the project as openly as


Most of the spec file is pretty standard

The handling of icon cache / bGConf / desktop file is slightly different 
from the standard scriplet snippets on the wiki, but in my opinion the 
differences aren't significant and I'm a little unwilling to change them since 
what is in the spec file now is pretty well tested. 

(Note on the desktop file ... not using desktop-file-install to add
the Fedora category to the desktop file intentionally because it's not 
very useful to start the client from the menu in most cases. It's added
to autostart folder on installation. But you could argue either way on that.)

The complexity in the spec file is all about handling the Firefox extension;
there is a very long comment explaining the logic. It seems to be working
pretty well right now, but certainly like any use of triggers, the more eyes
looking at it the better.
Comment 1 Owen Taylor 2006-10-24 12:17:04 EDT
Created attachment 139238 [details]
Mugshot Spec file
Comment 2 Michael Schwendt 2006-10-24 17:10:21 EDT
There's also a --makefile-uninstall-rule for %pre and %preun.
( http://fedoraproject.org/wiki/Packaging/ScriptletSnippets )
Comment 3 Owen Taylor 2006-11-06 14:04:36 EST
Created attachment 140497 [details]
Spec file with improved GConf scripts

New version with the the sophistication/complexity of the GConf 
handling increased to match ScripletSnippets.

(Quote from Havoc: 'Mostly this just makes me think "geez, someone 
should really take a couple days and move schemas to be loaded 
client-side already"')
Comment 4 Anthony Green 2006-11-11 21:35:16 EST
(In reply to comment #3)
> Created an attachment (id=140497) [edit]
> Spec file with improved GConf scripts

This spec file refers to a newer tarball.  Could you please post the updated srpm?
Comment 5 Owen Taylor 2006-11-12 10:10:17 EST

Is the current SRPM, which includes the improvements from the spec file above.
Comment 6 dff 2006-12-13 18:19:42 EST
This is irrelevant to the client software package review happening here, but
minor correction to the initial description: while the client software is 100%
GPL, the server software is primarily licensed under the OSL 3.0.  See
http://developer.mugshot.org/wiki/License for info.
Comment 7 Oisin C. Feeley 2007-01-04 12:07:08 EST
Apologies if this is the wrong place to report it but "yum localinstall
mugshot-1.1.30-1.fc5.i386.rpm" seems not to be signed:

# yum localinstall downloads/mugshot-1.1.30-1.fc5.i386.rpm
Loading "installonlyn" plugin
Setting up Local Package Process
Examining downloads/mugshot-1.1.30-1.fc5.i386.rpm: mugshot - 1.1.30-1.fc5.i386
Marking downloads/mugshot-1.1.30-1.fc5.i386.rpm to be installed
Setting up repositories
livna                                                                [1/5]
livna                     100% |=========================|  951 B    00:00
core                                                                 [2/5]
core                      100% |=========================| 1.1 kB    00:00
updates                                                              [3/5]
updates                   100% |=========================| 1.2 kB    00:00
freshrpms                                                            [4/5]
freshrpms                 100% |=========================|  951 B    00:00
extras                                                               [5/5]
extras                    100% |=========================| 1.1 kB    00:00
Reading repository metadata in from local files
Resolving Dependencies
--> Populating transaction set with selected packages. Please wait.
---> Package mugshot.i386 0:1.1.30-1.fc5 set to be updated
--> Running transaction check
--> Processing Dependency: loudmouth >= 1.0.3-3 for package: mugshot
--> Processing Dependency: libloudmouth-1.so.0 for package: mugshot
--> Restarting Dependency Resolution with new changes.
--> Populating transaction set with selected packages. Please wait.
---> Downloading header for loudmouth to pack into transaction set.
loudmouth-1.0.3-3.fc5.i38 100% |=========================| 4.3 kB    00:00
---> Package loudmouth.i386 0:1.0.3-3.fc5 set to be updated
--> Running transaction check

Dependencies Resolved

 Package                 Arch       Version          Repository        Size
 mugshot                 i386       1.1.30-1.fc5    
downloads/mugshot-1.1.30-1.fc5.i386.rpm  1.1 M
Installing for dependencies:
 loudmouth               i386       1.0.3-3.fc5      extras             53 k

Transaction Summary
Install      2 Package(s)
Update       0 Package(s)
Remove       0 Package(s)
Total download size: 1.2 M
Is this ok [y/N]: y
Downloading Packages:
(1/1): loudmouth-1.0.3-3. 100% |=========================|  53 kB    00:00

Package mugshot-1.1.30-1.fc5.i386.rpm is not signed
Comment 8 Owen Taylor 2007-01-04 12:13:32 EST
Yes... getting a recognized signature on the Mugshot RPM is one of the
main reasons we want to build the Mugshot Fedora packages in fedora-extras.
Along with building for all architectures.

We could sign the packages we build currently with a "Mugshot" GPG key,
but there really wouldn't be a lot of point to that, since nobody would
have that key on their system.

So once this review request clears the situation will be better (it's
currently blocking on some licensing details that we should have resolved
Comment 9 Anthony Green 2007-01-26 12:10:34 EST
Any update on the licensing issue?
Comment 10 Thomas Vander Stichele 2007-02-06 10:04:57 EST

what update is needed ? The client is GPL, and the server license is irrelevant
to a package of the client.  AFAICT this can proceed.
Comment 11 Anthony Green 2007-02-06 14:14:03 EST
(In reply to comment #10)
> Anthony,
> what update is needed ? The client is GPL, and the server license is irrelevant
> to a package of the client.  AFAICT this can proceed.

I believe the intent is for the client to be GPL, but it currently comes with an
EULA wrapper.   My understanding is that the mugshot team is working with RH
legal to remove the wrapper from the client package.  I'm just waiting for a
package update that removes the EULA text.

Comment 12 Owen Taylor 2007-03-15 19:10:51 EDT
OK, here's a new version with updated licensing (the package has also grown
some new build dependencies, but other than that I don't think the spec file
has changed significantly.)

The licensing is straight GPL, however to use the Mugshot trademarks, you
have to follow the Mugshot trademark guidelines found at:


These guidelines are as liberal as I could convince Red Hat legal was OK 
while still being able to maintain trademark; in rough summary:

 - Building the source tarball unmodified is OK
 - If you build a package with "normal" packaging modifications such  
   as fixing bugs or porting to new platforms, then you are required to
   add an "Adapted for <X>" notice to the about dialog. This is as 
   simple as adding a configure flag and is noted in comments in the
   spec file.
 - If you make significant changes to the user experience, or point the
   client at a different default server, then you cannot use the 
   Mugshot trademark.

I'll attach a new spec file. The corresponding source tarball referenced
in sources can be found at:

Comment 13 Owen Taylor 2007-03-15 19:12:02 EDT
Created attachment 150182 [details]
New version of the spec file
Comment 14 Anthony Green 2007-03-15 20:04:50 EDT
(In reply to comment #12)
> OK, here's a new version with updated licensing (the package has also grown
> some new build dependencies, but other than that I don't think the spec file
> has changed significantly.)

Thanks Owen.  I'll go through this carefully tonight or tomorrow.  In the
meanwhile, I can suggest a couple of changes.
1. Configure with --disable-static.  The current package includes .a and .la
files for the firefox thingy.  AFAICT these aren't needed and shouldn't be
2. Add "%config(noreplace)" in the %files section before the schema file like so...
%config(noreplace) %{_sysconfdir}/gconf/schemas/*.schemas
These changes silence rpmlint.
Comment 15 Peter Gordon 2007-03-15 21:26:59 EDT
(In reply to comment #14)
> 1. Configure with --disable-static.  The current package includes .a and .la
> files for the firefox thingy.  AFAICT these aren't needed and shouldn't be
> installed.

"--disable-static" will get rid of the .a files, but you probably will still
need to %exclude them in your %files section. (Globs are really nice for this.)

> 2. Add "%config(noreplace)" in the %files section before the schema file like
> %config(noreplace) %{_sysconfdir}/gconf/schemas/*.schemas
> These changes silence rpmlint.
Unfortunately, this one is a false positive from rpmlint. You would normally
change administrative GConf settings through gconf-editor or gconftool-2 by
their key/value pairs, and not by hand. See bug #223943 (comments 4 through 6)
for more details.

Comment 16 Anthony Green 2007-03-15 22:52:32 EDT
(In reply to comment #15)
> (In reply to comment #14)
> > 1. Configure with --disable-static.  The current package includes .a and .la
> > files for the firefox thingy.  AFAICT these aren't needed and shouldn't be
> > installed.
> "--disable-static" will get rid of the .a files, but you probably will still
> need to %exclude them in your %files section. (Globs are really nice for this.)

No need to exclude the .a file since it doesn't exist.  However I was wrong
about the .la file.  Either exclude that or manually delete the .la file at the
end of your %install.  FWIW I've been told by a few people that they prefer
manual deletion to exclusion.

Comment 17 Anthony Green 2007-03-16 20:07:37 EDT
Ok, here are my notes.  See the three lines starting with "X".  I would accept
this package once these minor issues are addressed.

* package meets and packaging guidelines.
* specfile is properly named, is cleanly written and uses macros consistently.
* dist tag is present.
* build root is correct.
* license field matches the actual license.
* license is open source-compatible.
* License text included in package.
* source files match upstream 
ef0f337dbbe919e0a67deac9591863ce  /usr/src/redhat/SOURCES/mugshot-1.1.38.tar.gz
ef0f337dbbe919e0a67deac9591863ce  mugshot-1.1.38.tar.gz
* latest version is being packaged.
* BuildRequires are proper.
* package builds in mock.

X rpmlint has two complaints.  See comments #14, #15 and #16.  I think we can
ignore the config file issue, but it would be good to clean up with .la and .a

* final provides and requires are sane:
  mugshot = 1.1.38-1
  loudmouth >= 1.0.3-3

* shared library is present, but no ldconfig required (firefox plugin).
* package is not relocatable.
* owns the directories it creates.
* doesn't own any directories it shouldn't.
* no duplicates in %files.
* file permissions are appropriate.
* %clean is present.
* %check is not present
* scriptlets OK
* code, not content.
* upstream includes no docs.
* %docs are not necessary for the proper functioning of the package.
* no headers.
* no pkgconfig files.
X includes libtool .la droppings.  This must be removed.
X .desktop looks fine, however...
  The packaging guidelines say that .desktop files MUST be installed with
desktop-file-install.  mugshot's Makefile simply copies the file instead.  If we
want to abide by the rules 100%, perhaps we should desktop-file-install the
.desktop file again.  Alternatively, we could run desktop-file-validate on the
Makefile installed .desktop file as a sanity check and fail the build if
necessary.  What do you think?  See
* not a web app.
Comment 18 Owen Taylor 2007-03-19 15:06:16 EDT
- I've added AM_DISABLE_STATIC svn mugshot and an rm for the .la file to my
  local copy of the spec file.

- I agree with Peter that the complaint about the schema files missing
  %config is a false positive; despite being in /etc, gconf schema 
  files are not configuration.

- desktop-file-install. Hmm. I disagree with the packaging guidelines:
   - The fedora- prefix is pointless and a misundestanding of the
     text about vendor prefixes in the menu specification (it would
     be the same thing as having all the binaries in /usr/bin start
     with fedora- ... the point of vendor prefixes is collision

   - The only thing that desktop-file-install here will do is validate
     the desktop file and force a prefix to be added.

   - The desktop file being installed is actually for autostart and
     not a menu desktop file under /usr/share/applications (adding it
     to the menu is intentionally not been done.)

  But that's really off-topic here... Luckily --vendor=mugshot does the
  right thing, so I'll just add to the spec file:

   desktop-file-install                                \
     --dir=$RPM_BUILD_ROOT%{_datadir}/gnome/autostart  \ 
     --vendor=mugshot                                  \

  so that we get the validation without mucking up the name. I don't
  want to use desktop-file-install in the Makefile since I don't see
  a point in putting a d-f-u dependency on the tarball build. 
  (As you say, desktop-file-validate would be a different option;
  Leaving it as desktop-file-install, maybe it will do something
  genuinely useful some day)

I'll attach a new version of the spec file, but all changes are described
above (with the spec file and 1.1.38, it will still package a .a file 
for the Firefox extension; there will be a 1.1.39 sometime in the next
few days with the AM_DISABLE_STATIC)

Thanks a lot for the careful review!
Comment 19 Owen Taylor 2007-03-19 15:08:06 EDT
Created attachment 150417 [details]
Tweaked spec file as described
Comment 20 Anthony Green 2007-03-20 11:44:59 EDT
(In reply to comment #18)
> - I've added AM_DISABLE_STATIC svn mugshot and an rm for the .la file to my
>   local copy of the spec file.

Why not configure with --disable-static in the spec file instead of waiting for
a 1.1.39?

In any case, this all looks good, so I think it makes sense to APPROVE this
package.  Thanks Owen!
Comment 21 Owen Taylor 2007-03-20 11:59:04 EDT
Well, there is at least one other thing in 1.1.38 that I want to fix before
putting a package into extras.... Given the opportunity, since the only 
person I'll be waiting on is myself, it seemed better to fix static in the 
package rather than the packaging. :-)

I'll proceed on now to subsequent steps in getting this built. Thanks again.
Comment 22 Warren Togami 2007-03-20 12:20:18 EDT
Please format a fedora-cvs request using the template.
Comment 23 Anthony Green 2007-03-20 12:26:17 EDT
Oops.  I'm still getting used to this new process...

New Package CVS Request
Package Name: mugshot
Short Description: Companion software for mugshot.org
Owners: otaylor@redhat.com
Branches: devel FC-6
Comment 24 Warren Togami 2007-03-20 12:44:19 EDT
Done.  otaylor will have access to import into the CVS branches after the next
ACL sync at 1:00PM EDT.  If he wants to grant CVS access to other people, he can
add their addresses to the pkg.acl file or delete the pkg.acl file.
Comment 25 Owen Taylor 2007-03-20 13:23:48 EDT
Can we get FC-5 and EL-5 branches added as well? Thanks
Comment 26 Anthony Green 2007-04-10 19:31:54 EDT
(In reply to comment #25)
> Can we get FC-5 and EL-5 branches added as well? Thanks

I think there was a mugshot rpm update a few days ago.  When will this show up
in rawhide?


Comment 27 Owen Taylor 2007-04-11 11:08:27 EDT
Right now, mugshot is excluded from being propagated to the Fedora extras
yum repository. This was done so that we could do testing before users got
upgraded to a new version of the client, and also coordinate client and
server pushes to happen at the same time.

But in retrospect, it's probably more confusing than it's worth, so we'll
likely request that this exclusion be removed.
Comment 28 Owen Taylor 2007-06-15 16:33:08 EDT
An F-7 branch is needed for mugshot.
Comment 29 Owen Taylor 2007-06-15 16:49:32 EDT
(Either someone got to this very quickly but hasn't cleared the flag yet, or
I somehow got confused and was unable to type cvs up correctly earlier.
A branch seems to exist now.)
Comment 30 Kevin Fenzi 2007-06-18 01:15:48 EDT
Yeah, I think it was always there... setting the flag to +
Comment 31 Anthony Green 2008-01-21 13:54:22 EST
This should have been closed a long time ago.  Closing now.

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