Bug 495412 (flamerobin) - Review Request: flamerobin - Graphical client for Firebird
Summary: Review Request: flamerobin - Graphical client for Firebird
Keywords:
Status: CLOSED NEXTRELEASE
Alias: flamerobin
Product: Fedora
Classification: Fedora
Component: Package Review
Version: 10
Hardware: All
OS: Linux
low
medium
Target Milestone: ---
Assignee: Peter Lemenkov
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On: 498790
Blocks:
TreeView+ depends on / blocked
 
Reported: 2009-04-12 20:45 UTC by Philippe Makowski
Modified: 2009-06-16 02:27 UTC (History)
4 users (show)

Fixed In Version: 0.9.2-0.fc11
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2009-06-11 07:16:15 UTC
Type: ---
Embargoed:
lemenkov: fedora-review+
j: fedora-cvs+


Attachments (Terms of Use)

Description Philippe Makowski 2009-04-12 20:45:09 UTC
Spec URL: http://ibphoenix.fr//fedora/flamerobin.spec
SRPM URL: http://ibphoenix.fr//fedora/flamerobin-0.9.2-0.fc10.src.rpm
Description: FlameRobin is a database administration tool for Firebird DBMS based on wxgtk toolkit

Comment 1 Zarko (grof) 2009-04-16 09:48:38 UTC
Hello, I am not a sponsor, so I can not take a sponsorship of you, but I can make an unofficial review.

At first, when writing a package from scratch, you should base your spec file on the Fedora spec file template, see:
http://fedoraproject.org/wiki/Rpmdevtools
command: rpmdev-newspec

For naming, licensing and other conventions, see&study Package Guidelines:
http://fedoraproject.org/wiki/Packaging/Guidelines

For using macros inside od spec, see:
http://fedoraproject.org/wiki/Packaging/RPMMacros

Build fails with this error:
usr/bin/ld: cannot find -lfbclient

please, fix this...

regards,
Zarko

Comment 2 Philippe Makowski 2009-04-18 18:31:00 UTC
Thanks
but it cannot be builded without firebird-devel :
BuildRequires:	firebird-devel

please re check it, I upgraded both spec and src.rpm

And that package is a Mandriva -> Fedora move, it is not writen from scratch ;)

Comment 3 Zarko (grof) 2009-04-18 20:52:22 UTC
(In reply to comment #2)

> 
> And that package is a Mandriva -> Fedora move, it is not writen from scratch ;)  

That is the problem. Some macros do not exist on Fedora.
Exp:
%{_iconsdir}/%{name}.png
%{_liconsdir}/%{name}.png
%{_miconsdir}/%{name}.png

I think that we do not have these macros on Fedora. Check it.

For desktop files you must do validate (it is commented for now)

I do not currently ttied build this package, but I will.
I see that the Firebird is in the review process, too, so I must build it in my personal repo for testing...

Comment 4 Philippe Makowski 2009-04-18 22:14:03 UTC
(In reply to comment #3)
> That is the problem. Some macros do not exist on Fedora.
> Exp:
> %{_iconsdir}/%{name}.png
> %{_liconsdir}/%{name}.png
> %{_miconsdir}/%{name}.png
> 
> I think that we do not have these macros on Fedora. Check it.
checked no problem
 
> For desktop files you must do validate (it is commented for now)
done

Comment 5 Zarko (grof) 2009-04-21 13:35:27 UTC
* Validation of .desktop files needs this:
  BuildRequires: desktop-file-utils
  so include this BR into spec

* Macro names:
  %{_iconsdir}
  %{_liconsdir}
  %{_miconsdir}

  do not pass in my mock build, please use standard macros from here:
  http://fedoraproject.org/wiki/Packaging/RPMMacros

Comment 6 Philippe Makowski 2009-04-21 21:13:32 UTC
sorry I really don't understand
if I make a rpm --showrc , these macros are there (f10)

and if not  %{_iconsdir},  %{_liconsdir},  %{_miconsdir}, then what else ?
Have you a suggestion ?

Comment 7 Zarko (grof) 2009-04-22 06:22:39 UTC
(In reply to comment #6)
> sorry I really don't understand
> if I make a rpm --showrc , these macros are there (f10)
> 
> and if not  %{_iconsdir},  %{_liconsdir},  %{_miconsdir}, then what else ?
> Have you a suggestion ?  

I currently have three Fedora installations on my PC Two of F10 and one of F11 branches

Only at one of my Fedora branches I have these macros defined, so I suggest to you that change these macros with:
 
%{_iconsdir} change to: %{_datadir}/icons
%{_liconsdir} change to: %{_datadir}/icons/large
%{_miconsdir} change to: %{_datadir}/icons/mini

Or, why you do not put these icons to standard dirs, like:

%{_datadir}/icons/hicolor/16x16/apps/%{name}.png
%{_datadir}/icons/hicolor/32x32/apps/%{name}.png
%{_datadir}/icons/hicolor/48x48/apps/%{name}.png

and reformat install and convert commands at %install section to use these dirs, and after that simply refresh icons cache?

Comment 8 Mamoru TASAKA 2009-04-22 14:25:03 UTC
Notes:

On my system _iconsdir or so is defined, however
-------------------------------------------------------
[tasaka1@localhost ~]$ grep %_iconsdir /etc/rpm/macros.*
/etc/rpm/macros.jpackage:%_iconsdir	%{_datadir}/icons
[tasaka1@localhost ~]$ grep -l %_iconsdir /etc/rpm/macros.* | xargs rpm -qf
jpackage-utils-1.7.5-2.7.fc11.noarch
-------------------------------------------------------
i.e. only when jpackage-utils (Java related package) is installed,
     these macros can be used.
So if you want to use macros such as _iconsdir, 
"BuildRequires: jpackage-utils" is needed to make it sure that
%_sysconfdir/rpm/macros.jpackage surely exists.

However
- This package does not seem to be related to Java
- So adding "BR: jpackage-utils" just to define %_iconsdir or
  so is not desired
- Also almost all packages on Fedora installing files under
  %_datadir/icons/ use %_datadir/icons, not %_iconsdir

So replacing %_iconsdir with %_datadir/icons is much preferable.

Comment 9 Philippe Makowski 2009-04-25 14:47:54 UTC
ok, upgraded
Spec URL: http://ibphoenix.fr//fedora/flamerobin.spec
SRPM URL: http://ibphoenix.fr//fedora/flamerobin-0.9.2-1.fc10.src.rpm

Comment 10 Peter Lemenkov 2009-05-03 12:11:32 UTC
Unblocking FE-NEEDSPONSOR - I sponsored Philippe.

Comment 11 Philippe Makowski 2009-05-07 21:25:30 UTC
since firebird is there now :

builds are ok for F-10 : 
http://koji.fedoraproject.org/koji/taskinfo?taskID=1341293

but can you help me to find why it failed for Epel ?
http://koji.fedoraproject.org/koji/getfile?taskID=1341334&name=build.log

Comment 12 Peter Lemenkov 2009-05-08 08:53:31 UTC
Please, don't raise fedora-review flags for packages submitted by you (this is a reviewer's work, not submitter's).

Comment 13 Philippe Makowski 2009-05-08 09:50:01 UTC
(In reply to comment #12)
> Please, don't raise fedora-review flags for packages submitted by you (this is
> a reviewer's work, not submitter's).  

oups sorry

Comment 15 Peter Lemenkov 2009-05-23 06:59:27 UTC
I'll review it.

Comment 16 Peter Lemenkov 2009-06-07 10:28:46 UTC
Notes:

* Use full patch for Source0:

http://dfn.dl.sourceforge.net/sourceforge/flamerobin/flamerobin-0.9.2-src.tar.gz

* remove --mandir explicit parameter passing (%configure already makes it)

* change %makeinstall to "make install DESTDIR=$RPM_BUILD_ROOT"

* this line is not needed at all "mkdir -p %{buildroot}/%{_datadir}/applications"

Other things looks sane. Here is a koji scratch build withall these changes:

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

Comment 18 Peter Lemenkov 2009-06-10 11:08:11 UTC
REVIEW:

+ rpmlint is almost silent - it only complains about empty sections, which should be removed (however it's a purely cosmetic change):

[petro@Workplace Desktop]$ rpmlint flamerobin-*
flamerobin.i386: W: empty-%post
flamerobin.i386: W: empty-%postun
flamerobin.i386: W: empty-%posttrans
2 packages and 0 specfiles checked; 0 errors, 3 warnings.
[petro@Workplace Desktop]$

+ The package is named according to the Package Naming Guidelines.
+ The spec file name matches the base package %{name}, in the format %{name}.spec.

+/- The package meets the Packaging Guidelines with one remaining issue - doc-files are listed twice. First - as %doc, second - as a contents of %{_datadir}/%{name}/docs. I advice you to add

rm -rf $RPM_BUILD_ROOT%{_datadir}/%{name}/docs

at the end of the %install section. Btw, maybe it's better to list docs as "%doc docs/*" rather than "%doc docs" ? 

+ The package is licensed with a Fedora approved license and meets the Licensing Guidelines.
+ The License field in the package spec file matches the actual license.
+ The  file, containing the text of the license(s) for the package, is included in %doc.
+ The spec file is written in American English.
+ The spec file for the package is legible.
+ The package successfully compiles and builds into binary rpms on at least one primary architecture. See links above.
+ All build dependencies are listed in BuildRequires.
+ A package owns all directories that it creates.
+ Permissions on files are set properly.
+ The package has a %clean section, which contains rm -rf %{buildroot} (or $RPM_BUILD_ROOT).
+ The package consistently uses macros. There is a single place where the $RPM_BUILD_ROOT is used instead of %{buildroot} - this may be ignored.
+ The package contains code, or permissible content.
+ No large documentation files.
+ Everything, the package includes as %doc, does not affect the runtime of the application.
+ The package includes a %{name}.desktop file, and that file is properly installed (checked with desktop-file-validate in the %install section).
+ The package does not own files or directories already owned by other packages. 
+ At the beginning of %install, the package runs rm -rf %{buildroot} (or $RPM_BUILD_ROOT).
+ All filenames in rpm packages are valid UTF-8.

Conclusion:

* consider to remove empty sections from spec-file
* remove duplicated docs from $RPM_BUILD_ROOT%{_datadir}/%{name}

Comment 19 Philippe Makowski 2009-06-10 13:49:27 UTC
ok
* consider to remove empty sections from spec-file
made
* remove duplicated docs from $RPM_BUILD_ROOT%{_datadir}/%{name}  
made

I also removed the no need  BuildRequires:ImageMagick


changes made :
Spec URL: http://ibphoenix.fr//fedora/flamerobin.spec

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

Comment 20 Peter Lemenkov 2009-06-10 14:03:20 UTC
Ok,

APPROVED.

Comment 21 Philippe Makowski 2009-06-10 15:47:17 UTC
New Package CVS Request
=======================
Package Name: flamerobin
Short Description: database administration tool for Firebird DBMS based on wxgtk
toolkit
Owners: pmakowski
Branches: F-10 F-11 EL-4 EL-5

Comment 22 Jason Tibbitts 2009-06-10 21:09:39 UTC
I see no account "pmakowski" in the account system.  Please correct, submit a new CVS request and set the fedora-cvs flag back to '?'.

Comment 23 Philippe Makowski 2009-06-10 22:17:03 UTC
sorry for the typo

New Package CVS Request
=======================
Package Name: flamerobin
Short Description: database administration tool for Firebird DBMS based on
wxgtk
toolkit
Owners: makowski
Branches: F-10 F-11 EL-4 EL-5

Comment 24 Jason Tibbitts 2009-06-10 22:47:28 UTC
CVS done.

Comment 25 Fedora Update System 2009-06-11 07:08:11 UTC
flamerobin-0.9.2-0.fc11 has been submitted as an update for Fedora 11.
http://admin.fedoraproject.org/updates/flamerobin-0.9.2-0.fc11

Comment 26 Fedora Update System 2009-06-11 07:08:17 UTC
flamerobin-0.9.2-0.fc10 has been submitted as an update for Fedora 10.
http://admin.fedoraproject.org/updates/flamerobin-0.9.2-0.fc10

Comment 27 Fedora Update System 2009-06-16 02:04:49 UTC
flamerobin-0.9.2-0.fc10 has been pushed to the Fedora 10 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 28 Fedora Update System 2009-06-16 02:27:02 UTC
flamerobin-0.9.2-0.fc11 has been pushed to the Fedora 11 stable repository.  If problems still persist, please make note of it in this bug report.


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