Created attachment 145381 [details]
initng-conf-gtk 0.4-1 spec file

I am not sponsor, so it is pre-review.
I am seeking for sponsor, at Bug 222594 .
I have looked at your spec file, and have some comments.

1) %makeinstall
You can not use %makeinstall. 
http://fedoraproject.org/wiki/Packaging/Guidelines#head-fcaf3e6fcbd51194a5d0dbcfbdd2fcb7791dd002

2) More macros
You can replace initng-conf-gtk with %{name}.

3) desktop-file-install --vendor fedora --delete-original \
  --dir $RPM_BUILD_ROOT%{_datadir}/applications \
  $RPM_BUILD_ROOT%{_datadir}/applications/initng-conf-gtk.desktop

--vendor fedora ->  --vendor "" 
You do not have to set vendor as fedora.

4) make
You have to use %{optflags} :
http://fedoraproject.org/wiki/Packaging/Guidelines#head-8b14098227aebff1cf6188939e9d0877295ac448

and also %{?_smp_mflags} :
http://fedoraproject.org/wiki/Packaging/Guidelines#head-525c7d76890cb22df33b759c65c35c82bf434d2e

5) Timestamp
I built SRPM, and found -p is not used for cp and install.
http://fedoraproject.org/wiki/Packaging/Guidelines#head-0239576e441f9ef53d175c4aec8c12868dffb5ab

(In reply to comment #2)
> 1) %makeinstall
> You can not use %makeinstall. 

Ok.

> 2) More macros
> You can replace initng-conf-gtk with %{name}.

Yep.

> 3) desktop-file-install --vendor fedora --delete-original \
>   --dir $RPM_BUILD_ROOT%{_datadir}/applications \
>   $RPM_BUILD_ROOT%{_datadir}/applications/initng-conf-gtk.desktop
> 
> --vendor fedora ->  --vendor "" 
> You do not have to set vendor as fedora.

Ah. Looks like I mixed up vendor_id and vendor...

> 4) make
> You have to use %{optflags} :
>
http://fedoraproject.org/wiki/Packaging/Guidelines#head-8b14098227aebff1cf6188939e9d0877295ac448

This fails with the following:

+ make -j2 -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions
-fstack-protector --param=ssp-buffer-size=4 -m64 -mtune=generic
make: invalid option -- O
make: invalid option -- 2
make: invalid option -- g
make: unrecognized option `--param=ssp-buffer-size=4'
make: invalid option -- 6
make: invalid option -- 4
make: invalid option -- u
make: invalid option -- =
make: invalid option -- g
make: invalid option -- c

Anyone knows what I'm doing wrong?

> and also %{?_smp_mflags} :
>
http://fedoraproject.org/wiki/Packaging/Guidelines#head-525c7d76890cb22df33b759c65c35c82bf434d2e

Yep.

> 5) Timestamp
> I built SRPM, and found -p is not used for cp and install.
>
http://fedoraproject.org/wiki/Packaging/Guidelines#head-0239576e441f9ef53d175c4aec8c12868dffb5ab

Now you lost me. I can't seem to find any cp or install commands in my spec file?

(In reply to comment #3)
> > 4) make
> > You have to use %{optflags} :
http://fedoraproject.org/wiki/Packaging/Guidelines#head-8b14098227aebff1cf6188939e9d0877295ac448
> This fails with the following:
<snip>
> Anyone knows what I'm doing wrong?
I checked build log, and it seems that %{optflag} is automatically set in your
case. %{optflag} is set in %configure..
I think you do not have to set %{optflags} here.
 
> > 5) Timestamp
> > I built SRPM, and found -p is not used for cp and install.
http://fedoraproject.org/wiki/Packaging/Guidelines#head-0239576e441f9ef53d175c4aec8c12868dffb5ab
> Now you lost me. I can't seem to find any cp or install commands in my spec file?
Yes, I lost. It was my mistake :-) 

In reply to comment #3)
> (In reply to comment #2)

> > 4) make
> > You have to use %{optflags} :
> >
>
http://fedoraproject.org/wiki/Packaging/Guidelines#head-8b14098227aebff1cf6188939e9d0877295ac448
> 
> This fails with the following:
> 
> + make -j2 -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions
> -fstack-protector --param=ssp-buffer-size=4 -m64 -mtune=generic
> make: invalid option -- O
> make: invalid option -- 2
> make: invalid option -- g
> make: unrecognized option `--param=ssp-buffer-size=4'
> make: invalid option -- 6
> make: invalid option -- 4
> make: invalid option -- u
> make: invalid option -- =
> make: invalid option -- g
> make: invalid option -- c
> 
> Anyone knows what I'm doing wrong?
You probably are facing a quoting bug somewhere when passing down $RPM_OPT_FLAGS
(In other words, some "" are missing somewhere).

(In reply to comment #3)
> This fails with the following:
> 
> + make -j2 -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions
> -fstack-protector --param=ssp-buffer-size=4 -m64 -mtune=generic
> make: invalid option -- O
> make: invalid option -- 2
> make: invalid option -- g
> make: unrecognized option `--param=ssp-buffer-size=4'
> make: invalid option -- 6
> make: invalid option -- 4
> make: invalid option -- u
> make: invalid option -- =
> make: invalid option -- g
> make: invalid option -- c
> Anyone knows what I'm doing wrong?
BTW, make CFLAGS="%{optflags}" 
will work.
In your case, you do not have to do so, because %configure does this.

Created attachment 146942 [details]
initng-conf-gtk 0.5-1 spec file

New upstreams release. Rolled new rpms according to the comments above.

New srpm at
http://download.initng.org/initng-gui/initng-conf-gtk/initng-conf-gtk-0.5-1.src.rpm

Ping. Someone?

it crashes for me when i start it up with the current initng / ifiles in extras fc6:

initng-conf-gtk: ngehandler.c:82: service_state: Zusicherung »serviceinfo« nicht
erfüllt.

This is not a formal review as I am not a sponsor:

* rpmlint:
 - W: invalid-license GPL
GPL is not a valid license, please use GPLv2[+] or GPLv3[+] instead. According
to the COPYING file and the source file's headers, the license is GPLv2+.

 - W: non-conffile-in-etc /etc/gconf/schemas/initng-conf-gtk.schemas
This can be ignored as gconf schema files always have to be overwritten on
package updates.

* The package is named according to the Package Naming Guidelines
* The spec file name matches the base package
* Fedora approved license (GPLv2+)
E License field in the package spec file DOES NOT match the actual license
* Latest version is being packaged
* Dist tag is present
* Build root is correct
* The sources used to build the package matches the upstream source
0dba2797764b33e1f58931a4ed11d465adc4d2ce  initng-conf-gtk-0.5.tar.gz
* The spec file is written in American English
* The spec file is legible
* Builds in mock (dist-f8)
http://koji.fedoraproject.org/koji/taskinfo?taskID=240809
* All build dependencies are listed in BuildRequires
* %install starts with build root clean
* %clean starts with build root clean
* Package own all directories that it creates
E Package owns files or directories already owned by other packages
/usr/share/icons/hicolor/16x16
/usr/share/icons/hicolor/16x16/apps
/usr/share/icons/hicolor/22x22
/usr/share/icons/hicolor/22x22/apps
/usr/share/icons/hicolor/24x24
/usr/share/icons/hicolor/24x24/apps
/usr/share/icons/hicolor/32x32
/usr/share/icons/hicolor/32x32/apps
/usr/share/icons/hicolor/36x36
/usr/share/icons/hicolor/36x36/apps
/usr/share/icons/hicolor/48x48
/usr/share/icons/hicolor/48x48/apps

I suggest replacing line 92 with
%{_datadir}/icons/hicolor/*/apps/initng.png
to solve this.

* GConf part looks OK
E Scriptlets are NOT sane:
- desktop-database part is not OK
Please update the affected parts according to 
http://fedoraproject.org/wiki/Packaging/ScriptletSnippets
The desktop file is installed properly.

- Scrollkeeper part is not OK

Update according to
http://fedoraproject.org/wiki/Packaging/ScriptletSnippets
as above

* No library
* Locale part handled properly
* Consolehelper part looks OK
* debuginfo package looks complete
* No duplicates in %files
* File permissions are appropriate
* The package contains code
* All filenames are valid UTF-8
* No pkgconfig files

Created attachment 259621 [details]
initng-conf-gtk 0.5-2 spec file

Thanks for your input. Actually I thought nobody cared about this review
request anymore. I've fixed the stuff that you notice. However the program
isn't really runnable right now anyway, because of the bug that Rudolf mentions
in #9. That has to be fixed upstreams, so I'll come back about it when I get
time...

Created attachment 259651 [details]
initng-conf-gtk 0.5.1-1 spec file

Ok, I pulled myself together and fixed the crash bug upstreams. Here is a spec
file that should produce a program that actually works.

Sorry for taking so long, Daniel.

You haven't posted a link to a new srpm file but I assume the old location is
still valid, hence I'm using
http://download.initng.org/initng-gui/initng-conf-gtk/initng-conf-gtk-0.5.1-1.fc8.src.rpm
for the continued review.

Following issues remain or are new:
* rpmlint (on srpm):
 - initng-conf-gtk.src: W: strange-permission initng-conf-gtk.spec 0755
Please remove the executable bit from the spec file.

* The sources used to build the package matches the upstream source
28b43e0dc8e5525a9c12c7f597f6e5829223f57d  initng-conf-gtk-0.5.1.tar.gz
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
E Timestamps:
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"
* No header files
* No static libs
* No library files
E Scriptlets are NOT sane (yet):
- gtk-update-icon-cache must be executed with '|| :' at the end (instead of ';')
so %post doesn't fail completely if something goes wrong

- update-desktop-database is missing in %post

E Desktop file handled properly (vendor id):
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.

E Whitespace in spec file:
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.

* Package does not own files or directories already owned by other packages
E Packages owns all directories that it creates:
file /usr/share/initng-conf-gtk is not owned by any package
Please replace
{_datadir}/%{name}/%{name}.glade
with
{_datadir}/%{name}


Aside from the issues left, the new source package in fact produces a working
program. 

Created attachment 289685 [details]
initng-conf-gtk 0.5.1-2 spec file

(In reply to comment #13)
> Sorry for taking so long, Daniel.

No problem. It isn't like I've been very active either ;-)
 
> You haven't posted a link to a new srpm file but I assume the old location is

> still valid, hence I'm using
>
http://download.initng.org/initng-gui/initng-conf-gtk/initng-conf-gtk-0.5.1-1.fc8.src.rpm

> for the continued review.

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


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

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

> - gtk-update-icon-cache must be executed with '|| :' at the end (instead of
';')
> so %post doesn't fail completely if something goes wrong

Fixed
 
> - update-desktop-database is missing in %post

Fixed
 
> 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.
 
> 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.
 
> Please replace
> {_datadir}/%{name}/%{name}.glade
> with
> {_datadir}/%{name}

Fixed
 
> Aside from the issues left, the new source package in fact produces a working

> program. 

Did that sound like you're surprised? ;-)

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.

(In reply to comment #15)
> again lots of time has passed but I'm going to continue the review now finally.

Nice. Don't really know how many people are interested anymore now that upstart
seems to be the preferred over initng in Fedora...

The new srpm is
http://download.initng.org/initng-gui/initng-conf-gtk/initng-conf-gtk-0.5.1-3.fc8.src.rpm

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

Yep, must've been a typo. Fixed now.

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

Fixed.

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

There is more to say. Not much for writing long texts though :-)
I wrote a bit longer sentence for the description now...

(In reply to comment #16)
> Nice. Don't really know how many people are interested anymore now that upstart
> seems to be the preferred over initng in Fedora...
I know, but it is always good to have alternatives and just because upstart is
there it doesn't mean initng will be useless.

> > 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.
> 
> There is more to say. Not much for writing long texts though :-)
> I wrote a bit longer sentence for the description now...
Well, now your (nice) description is too long for one line:
initng-conf-gtk.src: E: description-line-too-long

Description blocks in RPM SPECs must always break after 80 characters (including
newline, thus 79 readable chars).

Since you're using emacs you can make your life easier by adding to your ~/.emacs:

(add-hook 'rpm-spec-mode-hook
          (lambda ()
            (set-fill-column 79)))

(global-set-key "\M-q" 'fill-region-as-paragraph)

This often saves me a lot of time, especially for bigger SPEC files - take a
look at e.g. the latest SPEC for Bug 421241.

(In reply to comment #17)
> (In reply to comment #16)
> > Nice. Don't really know how many people are interested anymore now that upstart
> > seems to be the preferred over initng in Fedora...
> I know, but it is always good to have alternatives and just because upstart is
> there it doesn't mean initng will be useless.

Yep. I myself haven't used anything else but initng to boot my Linux machines
since initng 0.0.something and it's definitely doing it's thing good...

> > > 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.
> > 
> > There is more to say. Not much for writing long texts though :-)
> > I wrote a bit longer sentence for the description now...
> Well, now your (nice) description is too long for one line:
> initng-conf-gtk.src: E: description-line-too-long
> 
> Description blocks in RPM SPECs must always break after 80 characters (including
> newline, thus 79 readable chars).

Darn. Of course it'd have to be something like this when I for once didn't check
the files with rpmlint. Fixed it now though, the new srpm is
http://download.initng.org/initng-gui/initng-conf-gtk/initng-conf-gtk-0.5.1-4.fc8.src.rpm

(In reply to comment #18)
> (In reply to comment #17)
> > I know, but it is always good to have alternatives and just because upstart is
> > there it doesn't mean initng will be useless.
> 
> Yep. I myself haven't used anything else but initng to boot my Linux machines
> since initng 0.0.something and it's definitely doing it's thing good...
IIRC people are concerned over the way initng handles proper startup of init
scripts and lacking sysV init compatibility which upstart offers, hence the
decision was made to favor the latter.
But if initng floats one's boat, he or she should be able to install it.

> Darn. Of course it'd have to be something like this when I for once didn't check
> the files with rpmlint. Fixed it now though, the new srpm is
>
http://download.initng.org/initng-gui/initng-conf-gtk/initng-conf-gtk-0.5.1-4.fc8.src.rpm

Confirmed.

Also checked once more the package builds in mock for rawhide.
Package still owns all directories it creates and owns no directories from other
packages.

Small last fix before you cvs-import the package: Change "built in" in the
description to "built-in".

===============================
This package is APPROVED by me.
===============================

P.S. the German translation of initng-conf-gtk is pretty bad but that doesn't
matter for this review - I could help out with a better one, just drop me an email.

(In reply to comment #19)
> But if initng floats one's boat, he or she should be able to install it.

Good point. And it seems initng development has quite much stopped, so upstart
may have been the correct choice after all...

> Small last fix before you cvs-import the package: Change "built in" in the
> description to "built-in".

I'll do that. English is not my native language :-)

> ===============================
> This package is APPROVED by me.
> ===============================

Much appreciated!

> P.S. the German translation of initng-conf-gtk is pretty bad but that doesn't
> matter for this review - I could help out with a better one, just drop me an
email.

Sorry to hear that the translation is that bad. I must admit, I've never started
the program in german myself :-)
Any help in getting it any better is much appreciated. If you send me a better
one I'll include it in the next release (if there'll ever be one)...

New Package CVS Request
=======================
Package Name: initng-conf-gtk
Short Description: GTK configuration and control utility for initng
Owners: danielm
Branches: F-7 F-8
InitialCC: 
Cvsextras Commits: yes

cvs done.

Hmmm... I don't know if this is the correct place to ask, but the build failed
for F-8 and I have no idea what's wrong. I get the following:

exceptions.OSError: [Errno 17] File exists: '/mnt/koji/packages/initng-conf-gtk'

I guess this means I can't close this bug yet. Anyone got any idea what's wrong?

That's odd. I've checked your build (527348, right?) but there are no logs to
check. Maybe the parallel build with 527350 (dist-f9) caused the problem.. have
you tried to simply build again?

If nothing helps, maybe file a bug for mock or koji.

Yep. Building again did the trick. Didn't think there was any danger involved in
building multiple archs simultaneously...

Anyway, guess it's time to close this ticket now. Thank you all.