Bug 495412 (flamerobin)

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

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 ;)

(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...

(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

* 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

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 ?

(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?

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.

ok, upgraded
Spec URL: http://ibphoenix.fr//fedora/flamerobin.spec
SRPM URL: http://ibphoenix.fr//fedora/flamerobin-0.9.2-1.fc10.src.rpm

Unblocking FE-NEEDSPONSOR - I sponsored Philippe.

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

Please, don't raise fedora-review flags for packages submitted by you (this is a reviewer's work, not submitter's).

(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

I rewrite it

Spec URL: http://ibphoenix.fr//fedora/flamerobin.spec
SRPM URL: http://ibphoenix.fr//fedora/flamerobin-0.9.2-0.fc10.src.rpm  

and this time build is ok
http://koji.fedoraproject.org/koji/taskinfo?taskID=1349033
http://koji.fedoraproject.org/koji/taskinfo?taskID=1349026

I'll review it.

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

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

koji scratch builds are ok :
http://koji.fedoraproject.org/koji/taskinfo?taskID=1397855
http://koji.fedoraproject.org/koji/taskinfo?taskID=1397860
http://koji.fedoraproject.org/koji/taskinfo?taskID=1397864

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}

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

Ok,

APPROVED.

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

I see no account "pmakowski" in the account system.  Please correct, submit a new CVS request and set the fedora-cvs flag back to '?'.

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

CVS done.

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

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

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.

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.