Bug 448292

Hi Stefan,

This is not a formal review but only a pre-review of your package. I am not
entitled to Sponsor you however, someone else will.

1] Please perform rpmlint <rpmname> on every package that you create.

The following rpmlint errors must be resolved
---------------------------------------------------
bpanel.i386: E: arch-dependent-file-in-usr-share
/usr/share/fbpanel/plugins/dclock.so
fbpanel.i386: E: arch-dependent-file-in-usr-share
/usr/share/fbpanel/plugins/space.so
fbpanel.i386: E: arch-dependent-file-in-usr-share /usr/share/fbpanel/plugins/tray.so
fbpanel.i386: E: arch-dependent-file-in-usr-share
/usr/share/fbpanel/plugins/pager.so
fbpanel.i386: E: arch-dependent-file-in-usr-share
/usr/share/fbpanel/plugins/launchbar.so
fbpanel.i386: E: arch-dependent-file-in-usr-share
/usr/share/fbpanel/plugins/deskno2.so
fbpanel.i386: E: arch-dependent-file-in-usr-share
/usr/share/fbpanel/plugins/wincmd.so
fbpanel.i386: E: arch-dependent-file-in-usr-share /usr/share/fbpanel/plugins/test.so
fbpanel.i386: E: arch-dependent-file-in-usr-share
/usr/share/fbpanel/plugins/taskbar.so
fbpanel.i386: E: arch-dependent-file-in-usr-share /usr/share/fbpanel/plugins/cpu.so
fbpanel.i386: E: arch-dependent-file-in-usr-share
/usr/share/fbpanel/plugins/image.so
fbpanel.i386: E: arch-dependent-file-in-usr-share
/usr/share/fbpanel/plugins/tclock.so
fbpanel.i386: E: arch-dependent-file-in-usr-share
/usr/share/fbpanel/plugins/icons.so
fbpanel.i386: E: arch-dependent-file-in-usr-share
/usr/share/fbpanel/plugins/separator.so
fbpanel.i386: E: arch-dependent-file-in-usr-share /usr/share/fbpanel/plugins/menu.so
fbpanel.i386: E: arch-dependent-file-in-usr-share
/usr/share/fbpanel/plugins/deskno.so
----------------------------------------------------

2]Please note :

Every binary RPM package which stores shared library files (not just symlinks)
in any of the dynamic linker's default paths, must call ldconfig in %post and
%postun. If the package has multiple subpackages with libraries, each subpackage
should also have a %post/%postun section that calls /sbin/ldconfig. An example
of the correct syntax for this is:

%post -p /sbin/ldconfig

%postun -p /sbin/ldconfig

3] Packages containing GUI applications must include a %{name}.desktop file, and
that file must be properly installed with desktop-file-install in the %install
section. This is described in detail here
http://fedoraproject.org/wiki/Packaging/Guidelines#Desktop_files. If you feel
that your packaged GUI application does not need a .desktop file, you must put a
comment in the spec file with your explanation.

Thanks

Thank you Hemant for your feedback!
Here is my new version.

Spec URL: http://files.spielen-unter-linux.de/repository/fedora/9/SRPMS/
fbpanel.spec
SRPM URL: http://files.spielen-unter-linux.de/repository/fedora/9/SRPMS/
fbpanel-4.12-2.fc9.src.rpm

Changelog: - fixed rpmlint errors, new .desktop file, cleanup

Sorry for that, broken links....

http://cheekyboinc.spielen-unter-linux.de/fbpanel.spec
http://cheekyboinc.spielen-unter-linux.de/fbpanel-4.12-2.fc9.src.rpm

The srpm on comment 3 does not build.

http://koji.fedoraproject.org/koji/taskinfo?taskID=662736

Yes, fbpanel-4.12-libs.patch seems a bit broken, tries %{_libdir} rather 
$RPM_BUILD_ROOT%{_libdir} or so.

Stefan, you're replacing "$(PREFIX)/share/fbpanel/plugins" by "/usr/lib/fbpanel/
plugins" but you should try "(PREFIX)/lib/fbpanel/plugins" instead for example.

Oh, I forgot: This doesn't solve /usr/lib vs. /usr/lib64 anyway.

Created attachment 309404 [details]
fbpanel-4.12-libdir.patch

This patch should solve all issues regarding libraries in %{_datadir} and the
correct usage of %{_libdir} regarding /usr/lib vs. /usr/lib64. I think, I have
written it sane enough, so that maybe upstream also applies it.

SPEC: http://cheekyboinc.spielen-unter-linux.de/fbpanel.spec
SRPM: http://cheekyboinc.spielen-unter-linux.de/fbpanel-4.12-3.fc9.src.rpm

Changelog:

- Solved build failure and broken libs-patch with patch from Robert Scheck

Thank you Robert and Mamoru for your feedback!

If no one beats me, I will try to look at this package tomorrow.

For 4.12-3:

* License
  - Well, actually I cannot understand why the upstream ships MIT
    license text...
------------------------------------------------------------
bg.c			LGPLv2+
bg.h			LGPLv2+
ev.c			LGPLv2+
ev.h			LGPLv2+
gtkbar.c		LGPLv2+
gtkbar.h		LGPLv2+
gtkbgbox.c		LGPLv2+
gtkbgbox.h		LGPLv2+
plugins/cpu.c		GPLv2+
plugins/pager.c		GPLv2+
systray/eggtraymanager.c	LGPLv2+
systray/eggtraymanager.h	LGPLv2+
systray/fixedtip.c		GPLv2+
systray/fixedtip.h		GPLv2+
------------------------------------------------------------
    I have not analysed how these codes are used/linked yet,
    however from a quick glance, the license tag must be
------------------------------------------------------------
Group:		User Interface/Desktops
# %%{_bindir}/fbpanel and almost all plugins are under LGPLv2+
# Some plugins (cpu.so, pager.so, tray.so) are under GPLv2+
License:	LGPLv2+ and GPLv2+
URL:		http://fbpanel.sourceforge.net
------------------------------------------------------------

* SourceURL
  http://fedoraproject.org/wiki/Packaging/SourceURL#Sourceforge
  - Please follow above.

* SOURCE2
  - Write the full URL of SOURCE2, or write as a comment how
    you received Source2.

* Redundant BuildRequires
  - gtk2-devel Requires atk-devel, pango-devel, cairo-devel so
    all these 3 BuildRequires are redundant.

* Timestamp
  - When using "cp" or "install" commands, add "-p" option to
    keep timestamps on installed files, like
------------------------------------------------------------
install -p -m 0644 %{SOURCE2}	\
	$RPM_BUILD_ROOT%{_datadir}/icons/hicolor/32x32/apps
------------------------------------------------------------

  - Also, as this package installs many image files and so on
    which are actually not modified during build, keeping 
    timestamps on those files are also perferred.
    To do so, please consider to use below:
------------------------------------------------------------
sed -i.stamp -e 's|install -m|install -p -m|' \
	Makefile */Makefile
------------------------------------------------------------
    at %prep stage.

* Debuginfo rpm missing
  - rpmlint complains about debuginfo rpm as below:
------------------------------------------------------------
fbpanel-debuginfo.i386: E: empty-debuginfo-package
------------------------------------------------------------
    debuginfo rpm is not created correctly. Several reasons are
    related to this.
    * Fedora specific compilation flags are not honored correctly.
      Check:
      http://fedoraproject.org/wiki/Packaging/Guidelines#CompilerFlags
    ! To check if compiler flags are correctly honored, please
      make build log more verbosely. The outputs like
------------------------------------------------------------
    74  CC   systray/eggtraymanager.o
    75  DEP  plugins/space.dep
    76  DEP  plugins/pager.dep
    77  DEP  plugins/launchbar.dep
    78  CC   systray/fixedtip.o
    79  CC   plugin.o
    80  CC   gtkbar.o
 ------------------------------------------------------------
      is not useful.
    * Makefile "strip" binaries to be installed. /usr/lib/rpm/find-debuginfo.sh
      strips installed binaries correctly and Makefiles themselves must
      not strip binaries.
    * Installed plugin objects has 0644 permission. As these objects are dlopened
      so these should work, however as find-debuginfo.sh strips binaries only
      with executable permissions, on Fedora these must have 0755 permission.

    The following will fix these 4 issues
    (please check what these changes actually do)
-------------------------------------------------------------
%prep
%setup -q
%patch -p1 -b .libdir
sed -i.strip -e 's|strip|true strip|' \
	Makefile */Makefile
sed -i.stamp -e 's|install -m|install -p -m|' \
	Makefile */Makefile

%build
./configure --prefix=%{_prefix} --libdir=%{_lib}
make \
	%{?_smp_mflags} \
	Q= \
	CFLAGS="${RPM_OPT_FLAGS}"


%install
rm -rf $RPM_BUILD_ROOT
make install PREFIX=$RPM_BUILD_ROOT%{_prefix}
.......
.......
install -p -m 0644 %{SOURCE2}	\
	$RPM_BUILD_ROOT%{_datadir}/icons/hicolor/32x32/apps

find $RPM_BUILD_ROOT%{_libdir}/%{name} -name \*.so -print0 | \
	xargs -0 chmod 0755

%clean
.......
-------------------------------------------------------------

* /sbin/ldconfig call
  - Calling /sbin/ldconfig on scriptlets is not needed for this
    package as no "libraries" are installed under default ldconfig
    paths.

* Icon cache update
  - As icons are installed under %_datadir/icons/hicolor, please
    follow
    http://fedoraproject.org/wiki/Packaging/ScriptletSnippets#GTK.2B_icon_cache

Wow, so...
Here is the new package.

SPEC: http://cheekyboinc.spielen-unter-linux.de/fbpanel.spec
SRPM: http://cheekyboinc.spielen-unter-linux.de/fbpanel-4.12-4.fc9.src.rpm

Changelog:

- Add correct url for Source:
- Add gtk-update-icon-cache
- Add timestamps
- Add missing debuginfo rpm
- Changed licence, MIT to LGPLv2+ and GPLv2+
- Remove unneeded ldconfig
- Remove redundant BuildRequires: atk-devel, pango-devel and cairo-devel

For 4.12-4:

* License analysis
  - Please also write license analysis I wrote on comment 10
    as comments in the spec file.

* Again Source2:
--------------------------------------------------------
# image taken from the fbpanel source /config/images/star.png
Source2:	%{name}.png
--------------------------------------------------------
  - Okay, then you don't have to add this file seperately to
    srpm. The following will be enough.
--------------------------------------------------------
install -p -m 0644 config/images/star.png	\
	$RPM_BUILD_ROOT%{_datadir}/icons/hicolor/128x128/apps/%{name}.png
--------------------------------------------------------
    ! Note: This file has 128x128 size, not 32x32.

Then:
-------------------------------------------------------------
NOTE: Before being sponsored:

This package will be accepted with another few work. 
But before I accept this package, someone (I am a candidate) 
must sponsor you.

Once you are sponsored, you have the right to review other 
submitters' review requests and approve the packages formally. 
For this reason, the person who want to be sponsored (like you) 
are required to "show that you have an understanding 
of the process and of the packaging guidelines" as is described
on :
http://fedoraproject.org/wiki/PackageMaintainers/HowToGetSponsored

Usually there are two ways to show this.
A. submit other review requests with enough quality.
B. Do a "pre-review" of other person's review request
   (at the time you are not sponsored, you cannot do
   a formal review)

When you have submitted a new review request or have pre-reviewed other 
person's review request, please write the bug number on this bug report 
so that I can check your comments or review request.

Fedora package collection review requests which are waiting for someone to
review can be checked on:
http://fedoraproject.org/PackageReviewStatus/NEW.html
(NOTE: please don't choose "Merge Review")


Review guidelines are described mainly on:
http://fedoraproject.org/wiki/Packaging/ReviewGuidelines
http://fedoraproject.org/wiki/Packaging/Guidelines
http://fedoraproject.org/wiki/Packaging/ScriptletSnippets
------------------------------------------------------------

SPEC: http://cheekyboinc.spielen-unter-linux.de/fbpanel.spec
SRPM: http://cheekyboinc.spielen-unter-linux.de/fbpanel-4.12-5.fc9.src.rpm

Changelog:

- Add comment about the license
- Remove redundant Source2:

You can find another package (review request) here:

XQF - A server browser for many popular games
https://bugzilla.redhat.com/show_bug.cgi?id=451280

Thank you Mamoru for your helpfull overview about the review process and all 
the other things. 

Okay.

+ This package itself is good now
+ Your anothre review request seems good to some extent at a quick
  glance.

----------------------------------------------------------------
            This package (fbpanel) is APPOVED by me
----------------------------------------------------------------

Please follow the procedure written on:
http://fedoraproject.org/wiki/PackageMaintainers/Join
from "Get a Fedora Account".
At a point a mail should be sent to sponsor members which notifies
that you need a sponsor. At the stage, please also write on
this bug for confirmation that you requested for sponsorship and
your FAS (Fedora Account System) name. Then I will sponsor you.

If you want to import this package into Fedora 8/9, you also have
to look at
http://fedoraproject.org/wiki/Infrastructure/UpdatesSystem/Bodhi-info-DRAFT
(after once you rebuilt this package on koji Fedora rebuilding system).

If you have questions, please ask me.

I finished the procedure in May.
I am member of cla_done and have requested the membership of cvsextras.
My FAS name is "cheekyboinc"

------

Okay, I will look at the koji and Bodhi system! 

Now I am sponsoring you. please follow "Join" wiki again.

New Package CVS Request
=======================
Package Name: fbpanel
Short Description: a lightweight X11 desktop panel
Owners: cheekyboinc
Branches: F-8 F-9
InitialCC:
Cvsextras Commits: yes

cvs done.

fbpanel-4.12-5.fc9 has been submitted as an update for Fedora 9

fbpanel-4.12-5.fc8 has been submitted as an update for Fedora 8

Closing.

fbpanel-4.12-5.fc9 has been pushed to the Fedora 9 stable repository.  If problems still persist, please make note of it in this bug report.

fbpanel-4.12-5.fc8 has been pushed to the Fedora 8 stable repository.  If problems still persist, please make note of it in this bug report.

Package Change Request
======================
Package Name: fbpanel
New Branches: el6
Owners: splinux

Git done (by process-git-requests).

fbpanel-6.1-4.el6 has been submitted as an update for Fedora EPEL 6.
https://admin.fedoraproject.org/updates/fbpanel-6.1-4.el6

fbpanel-6.1-4.el6 has been pushed to the Fedora EPEL 6 stable repository.