Bug 222220 - Review Request: polyester - KDE style and window decoration
Summary: Review Request: polyester - KDE style and window decoration
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Chitlesh GOORAH
QA Contact: Fedora Package Reviews List
URL:
Whiteboard:
Depends On:
Blocks: FE-ACCEPT
TreeView+ depends on / blocked
 
Reported: 2007-01-10 22:07 UTC by Sebastian Vahl
Modified: 2007-11-30 22:11 UTC (History)
2 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2007-01-15 10:59:16 UTC
Type: ---
Embargoed:


Attachments (Terms of Use)

Description Sebastian Vahl 2007-01-10 22:07:47 UTC
Spec URL: http://www.deadbabylon.de/fedora/extras/polyester/polyester.spec
SRPM URL: http://www.deadbabylon.de/fedora/extras/polyester/polyester-1.0-1.fc6.src.rpm

Description: Widget style + kwin decoration both aimed to be
a good balance between eyecandy and simplicity.

Some notes: *.la-files cannot be removed without making this package unusuable.

Comment 1 Chitlesh GOORAH 2007-01-10 23:43:34 UTC
Ok, I'll do the review.

Comment 2 Sebastian Vahl 2007-01-11 10:12:20 UTC
(In reply to comment #1)
> Ok, I'll do the review.

Thx.

Comment 3 Josef Bacik 2007-01-11 19:51:54 UTC
==== REVIEW CHECKLIST ====
* rpmlint output
[josef@dhcp59-136 SPECS]$ 
rpmlint /home/josef/redhat/RPMS/x86_64/polyester-1.0-1.x86_64.rpm
E: polyester 
binary-or-shlib-defines-rpath /usr/lib64/kde3/kstyle_polyester_config.so 
['/usr/lib64']
E: polyester 
binary-or-shlib-defines-rpath /usr/lib64/kde3/kwin_polyester_config.so 
['/usr/lib64']
E: polyester binary-or-shlib-defines-rpath /usr/lib64/kde3/kwin3_polyester.so 
['/usr/lib64']
E: polyester 
binary-or-shlib-defines-rpath /usr/lib64/kde3/plugins/styles/polyester.so 
['/usr/lib64']

- package meets package naming guidelines
- spec filename matches %{name}
- package meets packaging guidelines
- package licensed with open source compatible license
- license matches actual license
- license file included in %doc
- spec written in American english
- spec file is legible
- sources match upstream
12e5f0fa6daeb083621635b410d7c813  polyester-1.0.tar.bz2
- package successfully compiles and builds on x86_64 FC6
- all build dependencies listed in BR
- locales handled properly
- uses ldconfig properly
- package is not relocatable
- package owns all directories it creates
- directories it does not create owned by default packages
- no duplicates in %files
- file permissions set properly
- contains proper %clean section
- macro usage consistent
- package contains code
- no large documentation
- files in %doc do not affect runtime
- no header files or static libraries
- no pkgconfig files
- no library files with suffix
- no need for devel subpackage
* contains .la files (should be OK 
http://fedoraproject.org/wiki/PackagingDrafts/LibtoolArchives)
* desktop file is not installed via the appropriate methods
- package does not own files or directories owned by other packages

====Things that need to be addressed====
* You need to fix the rpmlint errors.
* You do not need the "Requires: kdebase", that should be pulled in 
automatically
* You need to install the desktop file appropriatly, look here for an example
http://fedoraproject.org/wiki/Packaging/Guidelines#head-254ddf07aae20a23ced8cecc219d8f73926e9755
  gxine seems to install it by itself, so you will want to do something like 
what I did for geany
	http://www.toxicpanda.com/geany.spec

I'm not sponsored, this is just a review to help me get sponsored.


Comment 4 Rex Dieter 2007-01-11 19:58:34 UTC
FYI, desktop-file-install is not mandatory.  The guidelines only include
suggested usage.  (I mention this, mostly because I don't think it's use is
warranted here).

The rpath thing is annoying, especially having been built with ./configure
--disable-rpath.  ):

Comment 5 Christoph Wickert 2007-01-11 20:07:46 UTC
(In reply to comment #3)
> ==== REVIEW CHECKLIST ====
> ...
> * desktop file is not installed via the appropriate methods
> ...
> ====Things that need to be addressed====
> ...
> * You need to install the desktop file appropriatly, look here for an example
>
http://fedoraproject.org/wiki/Packaging/Guidelines#head-254ddf07aae20a23ced8cecc219d8f73926e9755

Josef, IMO you (and the guidelines) are wrong in this case. Please read the
following thread on FE-list:
https://www.redhat.com/archives/fedora-extras-list/2006-December/msg00494.html
especially
https://www.redhat.com/archives/fedora-extras-list/2006-December/msg00495.html and
https://www.redhat.com/archives/fedora-extras-list/2006-December/msg00496.html

Comment 6 Rex Dieter 2007-01-11 20:13:51 UTC
IMO, The Guidelines aren't wrong, they only say that .desktop files be installed
appropriately (though arguably that is too vague for some folks).

Comment 7 Sebastian Vahl 2007-01-11 20:23:47 UTC
Mhh. Strange. The problem with rpath doesn't exist on x86 and I don't have a
machine with x86_64 for testing. But I also don't know how to fix it.
--disable-rpath is already set, there seems no local copy of libtool and I am
not skilled in editing makefiles.

Any suggestions?


Comment 8 Dan Horák 2007-01-11 20:46:05 UTC
There is a local libtool in the "admin" subdirectory. You can add the following
lines (see
http://www.fedoraproject.org/wiki/Packaging/Guidelines#head-7cea8c7aa96400a4687e843156354476434ff883)
to the spec file after running configure and before running make.

# Don't use rpath!
sed -i 's|^hardcode_libdir_flag_spec=.*|hardcode_libdir_flag_spec=""|g' libtool
sed -i 's|^runpath_var=LD_RUN_PATH|runpath_var=DIE_RPATH_DIE|g' libtool

Only check where libtool is created whether in the toplevel source dir or in the
admin subdir and set the right path for libtool.

Comment 9 Christoph Wickert 2007-01-11 21:41:46 UTC
(In reply to comment #6)
> IMO, The Guidelines aren't wrong, they only say that .desktop files be installed
> appropriately (though arguably that is too vague for some folks).

ATM the guidelines state:
"Also remember that it is not simply enough to just include the .desktop file in
the package, you need to run desktop-file-install in %install "



Comment 10 Sebastian Vahl 2007-01-11 21:46:35 UTC
Oh, my fault. Didn't knowed that I must run %configure to get the libtool-file.
Thanks Dan.

Next try:
SPEC Url: http://www.deadbabylon.de/fedora/extras/polyester/polyester.spec
SRPM: Url:
http://www.deadbabylon.de/fedora/extras/polyester/polyester-1.0-2.fc6.src.rpm

Changelog:
- Removed kdebase from "Require"
- fix rpath 

Because I cannot reproduce the rpath error on x86 someone else must try it on
x86_64. Josef?

I also think so that desktop-file-install is not needed here.

Comment 11 Rex Dieter 2007-01-11 21:47:19 UTC
Sigh, ok, I guess we need to work to consolodate all the .desktop file
references into one place.

Comment 12 Rex Dieter 2007-01-11 21:50:49 UTC
More importantly, one should note that the example:
desktop-file-install --vendor=""                                 \
        --dir=%{buildroot}%{_datadir}/applications/<vendor_id>   \
        %{buildroot}/%{_datadir}/applications/<vendor_id>/foo.desktop

Is (almost) a no-op, other than serving as a sanity-check/validator for the
.desktop file (ie, dfi will barf if it finds something non-compliant).

Now, how to (re)word this to make it clear fdi is not a MUST item, while at the
same time maximizing confusion... (:


Comment 13 Sebastian Vahl 2007-01-11 22:55:20 UTC
Just a note to the conversation about the *.desktop-files: I've tried to avoid
packages which requires desktop-file-install in the past because I cannot say
I've really understanded this (usage/guidelines/syntax/etc.). :)


Comment 14 Chitlesh GOORAH 2007-01-13 13:05:30 UTC
Hello,
sorry about the dealy, I have some material problem at my place.
I'm trying to mock right now.

(In reply to comment #13)
> Just a note to the conversation about the *.desktop-files: I've tried to avoid
> packages which requires desktop-file-install in the past because I cannot say
> I've really understanded this (usage/guidelines/syntax/etc.). :)
> 

In this case, desktop-install-file isn't required since this is for kwin. 
But (for another package) however they should be used for GUI.

The package looks good.
Josef Whiter can you try rebuilding the package on x86_64 arch please?

Comment 15 Nicolas Chauvet (kwizart) 2007-01-13 16:57:57 UTC
I 've just rebuilt the release 2 on a x86_64 
rpmlint is clean...
There is no rpath issues

I should have used theses solutions:
http://fedoraproject.org/wiki/Extras/Schedule/RpathCheckBuildsys
But anyway this one needs also to be documented:
# Don't use rpath!
sed -i 's|^hardcode_libdir_flag_spec=.*|hardcode_libdir_flag_spec=""|g' libtool
sed -i 's|^runpath_var=LD_RUN_PATH|runpath_var=DIE_RPATH_DIE|g' libtool


Comment 16 Nicolas Chauvet (kwizart) 2007-01-13 17:01:30 UTC
http://kwizart.free.fr/fedora/6/testing/polyester/

Comment 17 Chitlesh GOORAH 2007-01-13 17:45:36 UTC
Thanks Nicolas Chauvet,

MUST Items:

- MUST: rpmlint's output is clean
- MUST: The package is named according to the Package Naming Guidelines.
- MUST: The spec file name matches the base package %{name}
- MUST: The package meets the Packaging Guidelines.
- MUST: The package is licensed (GPL) with an open-source compatible license
and meet other legal requirements as defined in the legal section of Packaging
Guidelines.
- MUST: The License field in the package spec file matches the actual license.
- MUST: the source package includes the text of the license(s) in its own file,
then that file, containing the text of the license(s) for the package is
included in %doc.
- MUST: The spec file must be written in American English.
- MUST: The spec file for the package is be legible. 
- MUST: The sources used to build the package must matches the upstream source,
as provided in the spec URL.
- MUST: The package successfully compiles and builds into binary rpms on at
least i386.
- MUST: All build dependencies is listed in BuildRequires.
- MUST: If the package does not contain shared library files located in the
dynamic linker's default paths
- MUST: the package is not designed to be relocatable
- MUST: the package owns all directories that it creates.
- MUST: the package does not contain any duplicate files in the %files listing.
- MUST: Permissions on files are set properly.
- MUST: The package has a %clean section, which contains rm -rf %{buildroot} (or
$RPM_BUILD_ROOT).
- MUST: The package consistently uses macros, as described in the macros section
of Packaging Guidelines.
- MUST: The package contains code, or permissable content. This is described in
detail in the code vs. content section of Packaging Guidelines.
- MUST: There are no Large documentation files
- MUST: %doc does not affect the runtime of the application. To summarize: If it
is in %doc, the program must run properly if it is not present.
- MUST: There are no Header files or static libraries 
- MUST: The package does not contain library files with a suffix 
- MUST: Package does not own files or directories already owned by other packages. 

SHOULD Items:
 - SHOULD: mock builds succcessfully in i386.
 - SHOULD: The reviewer tested that the package functions as described.
 - SHOULD: No scriptlets were used

APPROVED

Comment 18 Sebastian Vahl 2007-01-15 10:59:16 UTC
Imported. Thanks all for helping.


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