Bug 194279 - Review Request: kdeartwork: Extra KDE artwork (themes, sound themes, ...) for KDE
Summary: Review Request: kdeartwork: Extra KDE artwork (themes, sound themes, ...) for...
Keywords:
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Than Ngo
QA Contact: Fedora Package Reviews List
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2006-06-06 19:53 UTC by Rex Dieter
Modified: 2007-11-30 22:11 UTC (History)
4 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2007-06-20 14:43:51 UTC
Type: ---
Embargoed:
rdieter: fedora-review+


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

Description Rex Dieter 2006-06-06 19:53:16 UTC
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 11:46:59 UTC
%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 14:11:22 UTC
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 14:41:54 UTC
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 15:07:03 UTC
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 17:02:13 UTC
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 19:39:37 UTC
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 04:08:14 UTC
- 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 15:25:38 UTC
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 18:11:31 UTC
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 20:20:29 UTC
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 20:24:36 UTC
See also kdeartwork-extras review, bug #196379

Comment 12 Mike McGrath 2006-06-26 23:02:20 UTC
How often does upstream release packages that only have the icons changed?

Comment 13 Jason Tibbitts 2006-07-25 01:38:54 UTC
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 13:38:22 UTC
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 13:40:08 UTC
> 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 13:41:45 UTC
FYI, if it wasn't already obvious, this pkg is targetted for fc7.

Comment 17 Rex Dieter 2007-03-04 22:40:25 UTC
Mike?

Comment 18 Rex Dieter 2007-03-20 18:25:10 UTC
Than, were you going to finish this?

Comment 19 Than Ngo 2007-03-21 13:31:32 UTC
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 13:36:17 UTC
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 Than Ngo 2007-03-21 13:47:51 UTC
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 Than Ngo 2007-03-21 13:51:02 UTC
Created attachment 150574 [details]
new specfile

new specfile

Comment 23 Rex Dieter 2007-03-21 13:56:03 UTC
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 Than Ngo 2007-03-21 14:15:16 UTC
except the 2) it's fine with me. i approve this


Comment 25 Rex Dieter 2007-04-06 12:36:59 UTC
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 14:43:51 UTC
cvs updated...


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