Bug 212003

Created attachment 139238 [details]
Mugshot Spec file

There's also a --makefile-uninstall-rule for %pre and %preun.
( http://fedoraproject.org/wiki/Packaging/ScriptletSnippets )

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"')

(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?

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

Is the current SRPM, which includes the improvements from the spec file above.

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.

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
=============================================================================
Installing:
 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

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
soon.)

Any update on the licensing issue?

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.

(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.

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:

 http://mugshot.org/trademark

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:

 http://developer.mugshot.org/download/sources/linux/mugshot-1.1.38.tar.gz

Created attachment 150182 [details]
New version of the spec file

(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
installed.
2. Add "%config(noreplace)" in the %files section before the schema file like so...
%config(noreplace) %{_sysconfdir}/gconf/schemas/*.schemas
These changes silence rpmlint.

(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
so...
> %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.

Thanks.

(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.

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
files.

* final provides and requires are sane:
  libhippofirefox.so
  mugshot = 1.1.38-1
==
  GConf2
  gtk2
  libICE.so.6
  libORBit-2.so.0
  libSM.so.6
  libX11.so.6
  libXss.so.1
  libart_lgpl_2.so.2
  libatk-1.0.so.0
  libbonobo-2.so.0
  libbonobo-activation.so.4
  libbonoboui-2.so.0
  libcairo.so.2
  libcurl.so.3
  libdbus-1.so.3
  libdbus-glib-1.so.2
  libdl.so.2
  libgconf-2.so.4
  libgdk-x11-2.0.so.0
  libgdk_pixbuf-2.0.so.0
  libglib-2.0.so.0
  libgmodule-2.0.so.0
  libgnome-2.so.0
  libgnome-desktop-2.so.2
  libgnome-keyring.so.0
  libgnomecanvas-2.so.0
  libgnomeui-2.so.0
  libgnomevfs-2.so.0
  libgobject-2.0.so.0
  libgthread-2.0.so.0
  libgtk-x11-2.0.so.0
  libjpeg.so.62
  libloudmouth-1.so.0
  libm.so.6
  libnspr4.so
  libpango-1.0.so.0
  libpangocairo-1.0.so.0
  libpangoft2-1.0.so.0
  libpcre.so.0
  libplc4.so
  libplds4.so
  libpopt.so.0
  librt.so.1
  libstartup-notification-1.so.0
  libstdc++.so.6
  libstdc++.so.6(CXXABI_1.3)
  libstdc++.so.6(GLIBCXX_3.4)
  libxml2.so.2
  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
http://fedoraproject.org/wiki/Packaging/Guidelines#desktop
* not a web app.

- 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
     prevention.)

   - 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                                  \
     mugshot.desktop

  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!

Created attachment 150417 [details]
Tweaked spec file as described

(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!

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.

http://fedoraproject.org/wiki/PackageMaintainers/CVSAdminProcedure
Please format a fedora-cvs request using the template.

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
Branches: devel FC-6
InitialCC: 

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.

Can we get FC-5 and EL-5 branches added as well? Thanks

(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?

Thanks,

AG

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.

An F-7 branch is needed for mugshot.

(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.)

Yeah, I think it was always there... setting the flag to +

This should have been closed a long time ago.  Closing now.