Bug 988193 - Review Request: elementary-xfce-icon-theme - elementary-xfce-icon-theme
Review Request: elementary-xfce-icon-theme - elementary-xfce-icon-theme
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Christopher Meng
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2013-07-25 00:45 EDT by hannes
Modified: 2013-07-29 18:30 EDT (History)
5 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2013-07-29 08:46:49 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
i: fedora‑review+
limburgher: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description hannes 2013-07-25 00:45:38 EDT
Spec URL: http://hannes.fedorapeople.org/elementary-xfce-icon-theme.spec
SRPM URL: http://hannes.fedorapeople.org/elementary-xfce-icon-theme-0.3-1.fc19.src.rpm
Description: This is an icon-theme maintained with Xfce in mind,
but it supports other desktops like Gnome3 as well.
It's a fork of the upstream elementary-project, 
which took place because the team decided to
drop a lot of desktop-specific symlinks. 
This icon-theme is supposed to keep everything 
working, but we'll still pull new icons from upstream 
and integrate them occasionally.
Fedora Account System Username: hannes
Comment 1 Christopher Meng 2013-07-25 00:54:27 EDT
Please remove:

1. rm -rf  %{buildroot} in %install

2. whole %clean section.

3. %defattr(-,root,root,-)

%postun should be:

%postun
if [ $1 -eq 0 ] ; then
         touch --no-create %{_datadir}/icons/elementary-xfce &>/dev/null
         touch --no-create %{_datadir}/icons/elementary-xfce-dark &>/dev/null
         touch --no-create %{_datadir}/icons/elementary-xfce-darker &>/dev/null
         gtk-update-icon-cache -q %{_datadir}/icons/elementary-xfce &>/dev/null
         gtk-update-icon-cache -q %{_datadir}/icons/elementary-xfce-dark &>/dev/null
         gtk-update-icon-cache -q %{_datadir}/icons/elementary-xfce-darker &>/dev/null ||:
Comment 2 Christopher Meng 2013-07-25 00:55:26 EDT
%postun
if [ $1 -eq 0 ] ; then
         touch --no-create %{_datadir}/icons/elementary-xfce &>/dev/null
         touch --no-create %{_datadir}/icons/elementary-xfce-dark &>/dev/null
         touch --no-create %{_datadir}/icons/elementary-xfce-darker &>/dev/null
         gtk-update-icon-cache -q %{_datadir}/icons/elementary-xfce &>/dev/null
         gtk-update-icon-cache -q %{_datadir}/icons/elementary-xfce-dark &>/dev/null
         gtk-update-icon-cache -q %{_datadir}/icons/elementary-xfce-darker &>/dev/null ||:
fi

sorry.
Comment 3 hannes 2013-07-25 00:58:58 EDT
Ok, changed that. But I need to head to work now, so will have to do anything else later on today.
Thanks for the quick response!
http://hannes.fedorapeople.org/elementary-xfce-icon-theme.spec
SRPM URL: http://hannes.fedorapeople.org/elementary-xfce-icon-theme-0.3-2.fc19.src.rpm
Comment 4 Christopher Meng 2013-07-25 01:02:29 EDT
We don't need %posttrans now, right?
Comment 5 Raphael Groner 2013-07-25 03:37:18 EDT
(In reply to hannes from comment #3)
…
> Thanks for the quick response!
…
+1
Comment 7 Christopher Meng 2013-07-27 04:24:13 EDT
%post
touch --no-create %{_datadir}/icons/elementary-xfce &>/dev/null ||:
touch --no-create %{_datadir}/icons/elementary-xfce-dark &>/dev/null ||:
touch --no-create %{_datadir}/icons/elementary-xfce-darker &>/dev/null ||:

I think it should be

%post
touch --no-create %{_datadir}/icons/elementary-xfce &>/dev/null
touch --no-create %{_datadir}/icons/elementary-xfce-dark &>/dev/null
touch --no-create %{_datadir}/icons/elementary-xfce-darker &>/dev/null ||:
Comment 8 Michael Schwendt 2013-07-27 04:34:18 EDT
Why do you think that? Note that all three directories are included in the package.
Comment 9 Christopher Meng 2013-07-27 05:03:38 EDT
(In reply to Michael Schwendt from comment #8)
> Why do you think that? Note that all three directories are included in the
> package.

Just keep the same style with postun and posttrans.

Besides this package is approved now.
Comment 10 Michael Schwendt 2013-07-27 05:28:19 EDT
The scriptlets are wrong, though, and inconsistant.

> Just keep the same style with postun and posttrans.

Better forget about "style" and make them correct.

First of all, don't compare "touch" with "gtk-update-icon-cache". While touch is a coreutils tool, gtk-update-icon-cache may not be installed. That's why you want the scriptlets to _not_ fail when trying to run gtk-update-icon-cache when it isn't found. Secondly, in the odd case that "touch" is not available either, you could not touch the icon dirs, so the scriptlets would fail, too.

> %post
> touch --no-create %{_datadir}/icons/elementary-xfce &>/dev/null ||:
> touch --no-create %{_datadir}/icons/elementary-xfce-dark &>/dev/null ||:
> touch --no-create %{_datadir}/icons/elementary-xfce-darker &>/dev/null ||:

All three dirs are included in the package. And it's proper usage of "|| :" here for allow for a missing "touch" command.

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

Here, "|| :" should be added at the end of all lines. It makes no sense to add it to just the last line, because the previous two lines would fail already if gtk-update-icon-cache didn't exist. Similarly for "touch".


> %posttrans
>          gtk-update-icon-cache -q %{_datadir}/icons/elementary-xfce &>/dev/null
>          gtk-update-icon-cache -q %{_datadir}/icons/elementary-xfce-dark
&>/dev/null
>          gtk-update-icon-cache -q %{_datadir}/icons/elementary-xfce-darker
&>/dev/null ||:

Same here. "|| :" at the end of all lines would achieve what it's supposed to do.
Comment 11 Raphael Groner 2013-07-27 09:41:27 EDT
(In reply to Michael Schwendt from comment #10)
…
> First of all, don't compare "touch" with "gtk-update-icon-cache". While
> touch is a coreutils tool, gtk-update-icon-cache may not be installed.
> That's why you want the scriptlets to _not_ fail when trying to run
> gtk-update-icon-cache when it isn't found. Secondly, in the odd case that
> "touch" is not available either, you could not touch the icon dirs, so the
> scriptlets would fail, too.
…

I suggest to use 'if [ -x "$file" ]' therefore, where $file means '/usr/bin/touch' and '/usr/bin/gtk-update-icon-cache'. It could skip the whole touch or gtk-update-icon-cache calls individually cause of one false check at the beginning without trying several times to execute something that does not exist or is not marked as executable.
Comment 12 Raphael Groner 2013-07-27 09:53:32 EDT
(In reply to Raphael Groner from comment #11)
see also 
http://stackoverflow.com/questions/10319652/check-if-a-file-is-executable
http://www.tldp.org/LDP/Bash-Beginners-Guide/html/sect_07_01.html

Honestly, I would say that touch and gtk-update-icon-cache should be expected to be there on a good working Xfce system.

PS: What if bash is not installed? SCNR.
Comment 13 Michael Schwendt 2013-07-27 10:25:02 EDT
Do what works for you.

I'm not here to argue about which way to do it or whether a non-existant "touch" is worth supporting. I've only mentioned the purpose of those "|| :". The following page has been pointed at before:
https://fedoraproject.org/wiki/Packaging:ScriptletSnippets#Icon_Cache
Comment 14 Christopher Meng 2013-07-27 19:26:31 EDT
We don't need to care about bash, in fact they are installed as default in Fedora. Guideline has rules.
Comment 15 hannes 2013-07-28 02:26:38 EDT
New Package SCM Request
=======================
Package Name: elementary-xfce-icon-theme
Short Description: Icons for Xfce based on the elementary Project Icon Theme
Owners: hannes
Branches: f18 f19
Comment 16 Gwyn Ciesla 2013-07-29 08:13:00 EDT
Git done (by process-git-requests).
Comment 17 hannes 2013-07-29 08:46:49 EDT
Build in rawhide:
http://koji.fedoraproject.org/koji/buildinfo?buildID=438911
Comment 18 Michael Schwendt 2013-07-29 12:06:22 EDT
I need to correct myself, since I've been confused. It's the opposite, too many "|| :" in unneeded places, not missing "|| :".

> %post
> touch --no-create %{_datadir}/icons/elementary-xfce &>/dev/null ||:
> touch --no-create %{_datadir}/icons/elementary-xfce-dark &>/dev/null ||:
> touch --no-create %{_datadir}/icons/elementary-xfce-darker &>/dev/null ||:

To allow for a missing touch command, it would be enough to set the exit code at the end:

%post
touch --no-create %{_datadir}/icons/elementary-xfce &>/dev/null
touch --no-create %{_datadir}/icons/elementary-xfce-dark &>/dev/null
touch --no-create %{_datadir}/icons/elementary-xfce-darker &>/dev/null ||:
Comment 19 Christopher Meng 2013-07-29 18:30:48 EDT
(In reply to Michael Schwendt from comment #18)

;)

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