Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report. Summary: Review Request: initng-conf-gtk - InitNG configuration and control utility https://bugzilla.redhat.com/show_bug.cgi?id=222338 ------- Additional Comments From akahl@xxxxxxxxxxxxxx 2008-03-19 09:56 EST ------- Hello Daniel, again lots of time has passed but I'm going to continue the review now finally. (In reply to comment #14) > Quite right. And the new one now is > http://download.initng.org/initng-gui/initng-conf-gtk/initng-conf-gtk-0.5.1-2.fc8.src.rpm Using this one for the review continuation. > > - initng-conf-gtk.src: W: strange-permission initng-conf-gtk.spec 0755 > > Please remove the executable bit from the spec file. > > This one is strange. I'm quite sure I've never set the execute bit on the spec > file. Seems that the error is gone now though... Confirmed. > > E Consistent use of macros: > > Please use %{__rm} for rm and %{__ln_s} for 'ln -s', %{__mkdir} for mkdir, > > %{__make} for make, ${_bindir} for /usr/bin > > Fixed. Confirmed. > > To preserve timestamps on files upon copying, always add the -p option to > both > > cp and install, additionally change the 'make install' to > > make install DESTDIR=$RPM_BUILD_ROOT INSTALL="%{__install} -p" > > Fixed. Didn't find any cp or install in the spec file though... Yes, that was only a general information :) Confirmed. > > - gtk-update-icon-cache must be executed with '|| :' at the end (instead of > ';') > > so %post doesn't fail completely if something goes wrong > > Fixed Still wrong: You're using ${_bindir} instead of %{_bindir}, probably a typo only. Please fix it. > > - update-desktop-database is missing in %post > > Fixed Confirmed; however if I get http://fedoraproject.org/wiki/Packaging/ScriptletSnippets#head-de6770dd9867fcd085a73a4700f6bcd0d10294ef right, desktop-file-utils is needed in neither Requires(post) nor Requires(postun) anymore, so you can remove it from Requires(post). > > If a package specifies no vendor, use "fedora" as vendor id, see > > > http://fedoraproject.org/wiki/Packaging/Guidelines#head-d559ee7363418a5840ce63090c608c991cd39ce6 > > > Thus what Yuichi wrote in Comment #2 is wrong. > > Ah! I was sure I had seen that somewhere... Fixed again. Confirmed. > > > Although rpmlint doesn't complain about this, you're mixing up spaces and > tabs > > in your spec file scripts. Please use one of them consistently. > > Emacs doesn't like me. Think it's fixed now though. Confirmed (checked via M-x occur in emacs, you could use that too) > > Please replace > > {_datadir}/%{name}/%{name}.glade > > with > > {_datadir}/%{name} > > Fixed Confirmed. > > Aside from the issues left, the new source package in fact produces a working > > > program. > > Did that sound like you're surprised? ;-) Actually, no :) One small issue left: The description should say more than the summary, however if there's nothing more to say, finish the sentence with a period at least. I'm going to approve this package after you've fixed the remaining issues mentioned above. -- Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email ------- You are receiving this mail because: ------- You are the QA contact for the bug, or are watching the QA contact. _______________________________________________ Fedora-package-review mailing list Fedora-package-review@xxxxxxxxxx http://www.redhat.com/mailman/listinfo/fedora-package-review