Bug 507480 - Review Request: moblin-icon-theme - Moblin icon theme
Review Request: moblin-icon-theme - Moblin icon theme
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
low Severity medium
: ---
: ---
Assigned To: Rahul Sundaram
Fedora Extras Quality Assurance
:
Depends On:
Blocks: FedoraMoblin20
  Show dependency treegraph
 
Reported: 2009-06-22 18:35 EDT by Peter Robinson
Modified: 2013-03-13 01:45 EDT (History)
6 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-08-04 07:32:45 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
sundaram: fedora‑review+
tibbs: fedora‑cvs+


Attachments (Terms of Use)
updated spec to incorporate all changes from comment # 15 (2.45 KB, text/plain)
2009-08-03 10:51 EDT, Christoph Wickert
no flags Details

  None (edit)
Comment 2 Rahul Sundaram 2009-08-02 01:18:51 EDT
I will review this shortly
Comment 3 Rahul Sundaram 2009-08-02 01:29:00 EDT
How did you determine that the license is CC-By-SA? The COPYING file is empty. Where does create-icon-theme.sh come from?
Comment 4 Rahul Sundaram 2009-08-02 02:19:24 EDT
You should also drop the explicit requires on gtk2. 

https://fedoraproject.org/wiki/Packaging/Guidelines#Requires
Comment 5 Peter Robinson 2009-08-02 02:29:31 EDT
(In reply to comment #3)
> How did you determine that the license is CC-By-SA? The COPYING file is empty.
> Where does create-icon-theme.sh come from?  

There are a couple of different COPYING files. The one in the root of the tarball that is installed in the 0.7 version is as follows:

# more /usr/share/doc/moblin-icon-theme-0.7/COPYING

  Copyright (C) 2009 Intel Corporation

This work is licenced under the Creative Commons Attribution-Share Alike 3.0
United States License. To view a copy of this licence, visit
http://creativecommons.org/licenses/by-sa/3.0/ or send a letter to Creative
Commons, 171 Second Street, Suite 300, San Francisco, California 94105, USA.

The create-icon-theme.sh is also in the root of the tarball.
Comment 6 Peter Robinson 2009-08-02 02:35:39 EDT
(In reply to comment #4)
> You should also drop the explicit requires on gtk2. 
> 
> https://fedoraproject.org/wiki/Packaging/Guidelines#Requires  

The reason I added it because the rpm itself doesn't have libraries in it that can be used for determining requirements but the pre/post scripts use gtk-update-icon-cache which requires gtk2.
Comment 7 Rahul Sundaram 2009-08-02 02:37:31 EDT
Ah, I just realized I had downloaded the previous source instead of the latest.
Sorry for the confusion. 

You are APPROVED
Comment 8 Peter Robinson 2009-08-02 02:42:12 EDT
Thanks :-)

New Package CVS Request
=======================
Package Name: moblin-icon-theme
Short Description: Moblin icon theme
Owners: pbrobinson
Branches: F-11
InitialCC:
Comment 9 Christoph Wickert 2009-08-02 20:38:46 EDT
(In reply to comment #6)
 
> The reason I added it because the rpm itself doesn't have libraries in it that
> can be used for determining requirements but the pre/post scripts use
> gtk-update-icon-cache which requires gtk2.  

"Note that no dependencies should be added for this. If gtk-update-icon-cache is not available, there's nothing that would be needing the cache update, ..."
Read the whole paragraph at 
https://fedoraproject.org/wiki/Packaging/ScriptletSnippets#Icon_Cache

BTW: You are not preserving timestamps while copying the files, see
https://fedoraproject.org/wiki/Packaging/Guidelines#Timestamps
I'd rather use install instead of cp to make sure permissions are correct. If you want to use cp, please use %attr to verify the permissions.
Comment 10 Jason Tibbitts 2009-08-03 02:31:17 EDT
CVS done.
Comment 11 Rahul Sundaram 2009-08-03 02:50:24 EDT
Peter Robinson,

Please fix the issues noted by Christopher Wickert and then import
Comment 12 Peter Robinson 2009-08-03 03:55:21 EDT
> Please fix the issues noted by Christopher Wickert and then import  

I will when I actually get the time to read what's written :)
Comment 13 Peter Robinson 2009-08-03 06:35:04 EDT
I've updated the spec file using 'cp -p' as per the above guidelines it looks like from my reading and testing install is only usable for individual files rather than directory structure like in an icon theme.

I've also removed the Makefile.am files that are copied as well.

A side point I'm not sure how this script from the packaging guidelines works

%postun
if [ $1 -eq 0 ] ; then
    touch --no-create %{_datadir}/icons/hicolor &>/dev/null
    gtk-update-icon-cache %{_datadir}/icons/hicolor &>/dev/null || :
fi

As post uninstall the theme.index wouldn't exist. I think it needs a -t option.

Thanks for the pointers.
Comment 14 Peter Robinson 2009-08-03 06:36:58 EDT
> As post uninstall the theme.index wouldn't exist. I think it needs a -t option.

I meant index.theme
Comment 15 Christoph Wickert 2009-08-03 10:50:38 EDT
(In reply to comment #13)
> I've updated the spec file using 'cp -p' as per the above guidelines it looks
> like from my reading and testing install is only usable for individual files
> rather than directory structure like in an icon theme.

This is correct. Just stick with cp, create-icon-theme.sh takes care of the permissions. Sorry for the noise.

> As post uninstall the theme.index wouldn't exist. I think it needs a -t option.

No, if index.theme doesn't exist, gtk-update-icon-cache is not run - this is what we want. It will complain about the missing dir, but this is why we have " &>/dev/null || :" at the end. (Well, at least we *should* have this at the end, you only have "|| :" because your scriptlets are outdated. See 
https://fedoraproject.org/wiki/Packaging/ScriptletSnippets#Icon_Cache
for the most recent version.)

BTW: icon-theme.cache is not part of this package, so it wont get removed when the package uninstalled. You need to
touch %{buildroot}/%{_datadir}/icons/moblin/icon-theme.cache
during install and include it ghosted in the files section.

Another thing I realized while looking at the source: You are not creating any symlinks for the icons, so many apps will have no stock icons. Add the following to your spec at the end of %install, when the Makefile.am files are already removed:

# create symlinks for gtk stock icons, these are not really 'legacy'
# this uses the legacy-icon-mapping.xml file
cd $RPM_BUILD_ROOT/usr/share/icons/moblin
for size in 16x16 24x24 48x48; do
  (
  cd $size
  for context in *; do
    if [ -d $context ]; then
      (
      cd $context
      INU_DATA_DIR=%{_builddir}/%{name}-%{version} /usr/bin/icon-name-mapping -c $context
      )
    fi
  done
  )
done

Voila, now the icon theme contains more than 3 times more files.

Last but not least create-icon-theme.sh should be running during %build. Not that it makes a difference, I'm just pedantic. ;)
Comment 16 Christoph Wickert 2009-08-03 10:51:41 EDT
Created attachment 356041 [details]
updated spec to incorporate all changes from comment # 15
Comment 17 Christoph Wickert 2009-08-03 10:54:29 EDT
Well, all changes except preserving the timestamps. Sorry.
Comment 18 Peter Robinson 2009-08-03 11:21:04 EDT
Excellent, I'll have a look a this shortly. Thanks :)
Comment 19 Christoph Wickert 2009-08-03 13:53:40 EDT
One more thing: Please don't use a disttag here. This is a noarch theme with no dependencies, so it is not necessary to update it during an release upgrade.

As the package was already built today, you should untag the old version:
$ koji untag-pkg dist-f12 moblin-icon-theme-0.7-1.fc12
Comment 20 Peter Robinson 2009-08-03 15:59:06 EDT
Package updated.

http://koji.fedoraproject.org/koji/taskinfo?taskID=1577021
Comment 21 Christoph Wickert 2009-08-03 17:21:43 EDT
Looks good, thanks.

I just saw your commit in moblin's git. I didn't know that you are involved upstream. Sorry if I tried to explain you things you already know. ;)
Comment 22 Peter Robinson 2009-08-03 17:33:07 EDT
> I just saw your commit in moblin's git. I didn't know that you are involved
> upstream. Sorry if I tried to explain you things you already know. ;)  

I'm not involved :-). I'm packaging it up for Fedora and send patches for things that I find so I assume they use the git feature to assign the credit when they apply the patches.
Comment 23 Peter Robinson 2009-08-03 18:31:07 EDT
Built and on its way to rawhide.
Comment 24 Peter Robinson 2009-08-04 07:32:45 EDT
Built and now in rawhide

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