Bug 194279 - Review Request: kdeartwork: Extra KDE artwork (themes, sound themes, ...) for KDE
Review Request: kdeartwork: Extra KDE artwork (themes, sound themes, ...) for...
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Ngo Than
Fedora Package Reviews List
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2006-06-06 15:53 EDT by Rex Dieter
Modified: 2007-11-30 17:11 EST (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-06-20 10:43:51 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
rdieter: fedora‑review+


Attachments (Terms of Use)
new specfile (10.33 KB, application/octet-stream)
2007-03-21 09:51 EDT, Ngo Than
no flags Details

  None (edit)
Description Rex Dieter 2006-06-06 15:53:16 EDT
Spec URL: http://kde-redhat.unl.edu/apt/kde-redhat/SPECS/kdeartwork.spec
SRPM URL: http://kde-redhat.unl.edu/apt/kde-redhat/all/SRPMS.testing/kdeartwork-3.5.3-2.src.rpm
Description:
Additional artwork (themes, sound themes, ...) for KDE
Comment 1 Rex Dieter 2006-06-07 07:46:59 EDT
%changelog
* Mon Jun 05 2006 Rex Dieter <rexdieter[AT]users.sf.net> 3.5.3-2
- cleanup for Extras
- %%doc: README
Comment 2 Mike McGrath 2006-06-14 10:11:22 EDT
To get this to build I had to remove %{_datadir}/apps/kfiresaver/ for some
reason its not getting built/installed

- add "BuildRequires: gettext" for translations
- User Interface/Desktop isn't a valid group name, unless its new to FC6?
- Under setup the %{?beta} tag is used, is this intentional?
- Do any of these apps require .desktop files?
Comment 3 Rex Dieter 2006-06-14 10:41:54 EDT
Re: comment #2:

>To get this to build I had to remove %{_datadir}/apps/kfiresaver/ for some
> reason its not getting built/installed
Probably due to qt bug #193741
I could probably workaround that here, for now, by adding 
BR: libGL-devel libGLU-devel

> - add "BuildRequires: gettext" for translations
AFAICT, there are no translations here (those bits are in kde-i18n-* pkgs), so
gettext ought not be needed.

> - User Interface/Desktop isn't a valid group name, unless its new to FC6?
That's what Core's kdeartwork package uses.  Do you have a better suggested value?

> - Under setup the %{?beta} tag is used, is this intentional?
I define the beta macro when/if there are any alpha/beta/preleleases done by
kde.  %{?beta} evaluates to %{nil} when undefined.  I could probably remove
that, since it's hardly used anymore.

> - Do any of these apps require .desktop files?
Apps no, screensaver's yes, in %%_datadir/applnk/System/Screensavers/
Comment 4 Rex Dieter 2006-06-14 11:07:03 EDT
Spec URL: http://kde-redhat.unl.edu/apt/kde-redhat/SPECS/kdeartwork.spec
SRPM URL:
http://kde-redhat.unl.edu/apt/kde-redhat/all/SRPMS.testing/kdeartwork-3.5.3-4.src.rpm

%changelog
* Wed Jun 14 2006 Rex Dieter <rexdieter[AT]users.sf.net> 3.5.3-4
- BR: libGL-devel libGLU-devel (workaround qt bug #193741)
- Group: System Environment/Desktop -> Amusements/Graphics
- drop unused %%?beta macro
Comment 5 Rex Dieter 2006-06-16 13:02:13 EDT
Spec URL: http://kde-redhat.unl.edu/apt/kde-redhat/SPECS/kdeartwork.spec
SRPM URL:
http://kde-redhat.unl.edu/apt/kde-redhat/all/SRPMS.stable/kdeartwork-3.5.3-5.src.rpm

%changelog
* Fri Jun 16 2006 Rex Dieter <rexdieter[AT]users.sf.net> 3.5.3-5
- patch for kdeclassic missing Inherits=  (even though it *should* already
  implicity Inherits=hicolor ?)
Comment 6 Rex Dieter 2006-06-16 15:39:37 EDT
FYI, see also bug #195480, regarding kdeartwork's dep on xscreensaver.  Maybe 
splitting-out the xsc-dependant bits will be a reasonable option. 
Comment 7 Mike McGrath 2006-06-18 00:08:14 EDT
- Name looks good
- Source matches upstream
- Builds fine in mock against devel
- rpmlint:
  SRPM: 
      E: kdeartwork unknown-key GPG#ff6382fa
  kdeartwork:
      CLEAN
  kdeartwork-icons:
      W: kdeartwork-icons no-documentation
      E: kdeartwork-icons standard-dir-owned-by-package /usr/share/icons

One thing.  right now this package owns /usr/share/icons/  Aparently it's not alone.

rpm -qf /usr/share/icons

My box shows 6 packages owning that directory.  The guidelines state that your
package must own all directories it creates and must not own any directories
that another package creates.  My suggestion is to go ahead and take out
ownership of that directory and file a bugzilla for the filesystem package.  If
they refuse with a valid reason then I say go ahead and own it.

I'll wait for your response (and others) before I approve the package but aside
from the ownership issue this package is ready to go, that is of course
depending on what you decide to do for #195480
Comment 8 Rex Dieter 2006-06-19 11:25:38 EDT
FYI, see Bugzilla bug #195911 – "filesystem: own %_datadir/icons"

Since bug #195480 was CLOSED->NOTABUG, looks like we'll end up making a
kdeartwork-extras (sub?)pkg that contains (at least) the missing xscreensaver bits.
Comment 9 Rex Dieter 2006-06-19 14:11:31 EDT
Package split complete.  If need be, I can submit (some of) these under separate
reviews... at this point, the only one considerable for Extras is kdeartwork-extras.

Spec URL: http://kde-redhat.unl.edu/apt/kde-redhat/SPECS/kdeartwork.spec
SRPM URL:
http://kde-redhat.unl.edu/apt/kde-redhat/all/SRPMS.stable/kdeartwork-3.5.3-6.src.rpm

Spec URL: http://kde-redhat.unl.edu/apt/kde-redhat/SPECS/kdeartwork-extras.spec
SRPM URL:
http://kde-redhat.unl.edu/apt/kde-redhat/all/SRPMS.stable/kdeartwork-extras-3.5.3-6.src

Spec URL: http://kde-redhat.unl.edu/apt/kde-redhat/SPECS/kdeartwork-icons.spec
SRPM URL:
http://kde-redhat.unl.edu/apt/kde-redhat/all/SRPMS.stable/kdeartwork-icons-3.5.3-6.src

%changelog
* Mon Jun 19 2006 Rex Dieter <rexdieter[AT]users.sf.net> 3.5.3-6
- main(core), -icons, -extras subpkgs.
  -icons so we don't push ~11MB data on every pkg respin, and leave ability
  to someday make it .noarch
  -extras for xscreensaver bits (see bug #195480)
Comment 10 Rex Dieter 2006-06-22 16:20:29 EDT
I'll go ahead and submit kdeartwork-extras separately, since we can target that
for fc6+, sooner than the rest.
Comment 11 Rex Dieter 2006-06-22 16:24:36 EDT
See also kdeartwork-extras review, bug #196379
Comment 12 Mike McGrath 2006-06-26 19:02:20 EDT
How often does upstream release packages that only have the icons changed?
Comment 13 Jason Tibbitts 2006-07-24 21:38:54 EDT
Is this under review?  It's assigned, but still blocking FE-NEW.  If someone is
reviewing it, it should block FE-REVIEW instead.
Comment 14 Rex Dieter 2006-07-26 09:38:22 EDT
Mike said:
> How often does upstream release packages that only have the icons changed?

rarely (I won't say never), but the code portions of kdeartwork is periodically
patched.
Comment 15 Rex Dieter 2006-07-26 09:40:08 EDT
> Is this under review?  It's assigned, but still blocking FE-NEW.

I'll set it to FE-REVIEW.  Mike's free to change that back (and remove assigment
of this bug) if he feels otherwise.  (:
Comment 16 Rex Dieter 2006-07-26 09:41:45 EDT
FYI, if it wasn't already obvious, this pkg is targetted for fc7.
Comment 17 Rex Dieter 2007-03-04 17:40:25 EST
Mike?
Comment 18 Rex Dieter 2007-03-20 14:25:10 EDT
Than, were you going to finish this?
Comment 19 Ngo Than 2007-03-21 09:31:32 EDT
It's bad to have 3 SRPMS. You duplicate the sources! we should make only one
source rpm, and split in kdeartwork, kdeartworks-icons, kdeartworks-extras.

I have built new kdeartworks-3.5.6-2.fc7 in rawhide. Could you please take a
look at the new specfile? Thanks
Comment 20 Rex Dieter 2007-03-21 09:36:17 EDT
for the record, the reasons for separate srpms in the past were:
1.  kdeartwork-extras: needed for Extras, duh. :)
2.  kdeartwork-icons as proposed was to be .noarch.  I know of no other way to 
build a main .i386 pkg and a separate/sub .noarch pkg.

that being said, do you see anything else you see worth changing?  If not, 
feel free to approve this review.
Comment 21 Ngo Than 2007-03-21 09:47:51 EDT
1) for fc7, FC and Extras will be merged. therefore it should not be separate
package!

2) why don't you want to build it as noarch?
the problem is that we duplicate the source code (3x16Mb).
i don't see the benefit here.

please take a look at the new specfile. It looks cleaner. We could use the new
specfile. 
Comment 22 Ngo Than 2007-03-21 09:51:02 EDT
Created attachment 150574 [details]
new specfile

new specfile
Comment 23 Rex Dieter 2007-03-21 09:56:03 EDT
1) of course, F7+ doesn't need a separate pkg, that was only for FC-6 :)
2) we can debate this topic later, I don't see this affecting the package review.

I agree we should use the new specfile, but *you* are the reviewer here... :)
so it's your call on whether further changes are warranted.to pass review.
Comment 24 Ngo Than 2007-03-21 10:15:16 EDT
except the 2) it's fine with me. i approve this
Comment 25 Rex Dieter 2007-04-06 08:36:59 EDT
marked approved, per comment #24.

please update in cvs (I don't have access yet, poo), and then you can close this.
Comment 26 Rex Dieter 2007-06-20 10:43:51 EDT
cvs updated...

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