Bug 220969

Hi! Nice to see that another Pole wants to put his package in Extras! :-)
However there's a lot to do in your spec file.
 * First of all mock build of your package fails due to a simple mistake. You
put desktop-file-utils as Requires but it should be BuildRequires.
 * You don't have to explicitly set a version of gtk2 in requires. RPM should
build fine without it.
 * Your %files section doesn't look good. Package owns files in
%{_datadir}/%{name}/icons/ but doesn't own the parent one. It means that if you
remove RPM, all files within %{_datadir}/%{name}/icons/ will be deleted but the
dir remains. In order to fix it you ought to simply remove all
%{_datadir}/%{name}/icons/ lines and replace them with a simple
%{_datadir}/%{name}
 * I don't see it as a blocker but in my opinion much better solution would be
if you move a desktop file to another Source instead of creating it in spec.
That should be a lot more legible.

And the last thing: SPEC files are different at the URL you passed here and
inside SRPM. I hope you'll get rid of that issue in next release ;-)

PS. I also added FE-NEW blocker as it were missing.

(In reply to comment #1)
> Hi! Nice to see that another Pole wants to put his package in Extras! :-)
> However there's a lot to do in your spec file.

No-one is perfect ;)

>  * First of all mock build of your package fails due to a simple mistake. You
> put desktop-file-utils as Requires but it should be BuildRequires.

I moved it there due to:

desktop-file-install --vendor fedora \
    --dir %{buildroot}%{_datadir}/applications \
    %{name}.desktop

in %install section. But If mock doesn't like it I moved it back to BuildRequires.

>  * Your %files section doesn't look good. Package owns files in
> %{_datadir}/%{name}/icons/ but doesn't own the parent one. It means that if
> you remove RPM, all files within %{_datadir}/%{name}/icons/ will be deleted 
> but the dir remains. In order to fix it you ought to simply remove all
> %{_datadir}/%{name}/icons/ lines and replace them with a simple
> %{_datadir}/%{name}

Indeed. I missed it bacause I usually update my packages rather than remove :)

>  * I don't see it as a blocker but in my opinion much better solution would be
> if you move a desktop file to another Source instead of creating it in spec.
> That should be a lot more legible.

Ok, I moved it.

> And the last thing: SPEC files are different at the URL you passed here and
> inside SRPM. I hope you'll get rid of that issue in next release ;-)

This time I did my best to not update descriptions at the last moment :)


Thanks for you suggestions. 0.6-3 is ready for a review:
http://timeoff.wsisiz.edu.pl/zrzut/isomaster.spec
http://timeoff.wsisiz.edu.pl/zrzut/isomaster-0.6-3.src.rpm

(In reply to comment #2)
> I moved it there due to:
> 
> desktop-file-install --vendor fedora \
>     --dir %{buildroot}%{_datadir}/applications \
>     %{name}.desktop
> 
> in %install section. But If mock doesn't like it I moved it back to 
BuildRequires.
> 

Remember that %install section is executed at building an SRPM, not at
installing the binary one. That's why you need to put it into BR.
> >  * I don't see it as a blocker but in my opinion much better solution would 
be
> > if you move a desktop file to another Source instead of creating it in spec.
> > That should be a lot more legible.
> 
> Ok, I moved it.
> 

Desktop files aren't as large to put them into tarball. You can remove a tar
compression and get rid of second %setup macro. Now, you can put %{SOURCE1}
macro into desktop-file-install:
desktop-file-install --vendor fedora \
	--dir %{buildroot}%{_datadir}/applications \
	%{SOURCE1}

and it looks much better :)

(In reply to comment #3)
> Remember that %install section is executed at building an SRPM, not at
> installing the binary one. That's why you need to put it into BR.

Sure, it was logical mistake.

> Desktop files aren't as large to put them into tarball. You can remove a tar
> compression and get rid of second %setup macro. Now, you can put %{SOURCE1}
> macro into desktop-file-install:
> desktop-file-install --vendor fedora \
> 	--dir %{buildroot}%{_datadir}/applications \
> 	%{SOURCE1}

Yesterday I remembered why I had switched to generated .desktop files. In
prebuilt .desktop file there has to be fixed path to icon which can be incorrent
if package isn't installed in /usr. 
But it isn't probably a common problem.

> and it looks much better :)

Hopefully good enough to find a sponsor :)

Updated version:
http://timeoff.wsisiz.edu.pl/zrzut/isomaster.spec
http://timeoff.wsisiz.edu.pl/zrzut/isomaster-0.6-4.src.rpm

I tried to run rpmlint for your newest package and I got:
E: isomaster description-line-too-long ISO Master: an easy to use GUI CD image 
editor. It allows to extract files from an ISO, add files to an ISO, and create 
bootable ISOs - all in a graphical user interface.
E: isomaster description-line-too-long ISO Master: an easy to use GUI CD image 
editor. It allows to extract files from an ISO, add files to an ISO, and create 
bootable ISOs - all in a graphical user interface.
W: isomaster macro-in-%changelog _datadir

In order to get rid of the first two ones you have to split your %description.
One line inside it may be max 79 characters long.
To remove a warning just double % character, so instead of %{_datadir} write
%%{_datadir} etc.
Also I noticed that gtk2 dependency ought to be removed completely.
Running rpm -qpR for RPM gives me following result:
gtk2
libatk-1.0.so.0()(64bit)
libc.so.6()(64bit)
libc.so.6(GLIBC_2.2.5)(64bit)
libc.so.6(GLIBC_2.3)(64bit)
libcairo.so.2()(64bit)
libdl.so.2()(64bit)
libgdk-x11-2.0.so.0()(64bit)
libgdk_pixbuf-2.0.so.0()(64bit)
libglib-2.0.so.0()(64bit)
libgmodule-2.0.so.0()(64bit)
libgobject-2.0.so.0()(64bit)
libgtk-x11-2.0.so.0()(64bit)
libm.so.6()(64bit)
libpango-1.0.so.0()(64bit)
libpangocairo-1.0.so.0()(64bit)
rpmlib(CompressedFileNames) <= 3.0.4-1
rpmlib(PayloadFilesHavePrefix) <= 4.0-1
rtld(GNU_HASH)

As you can see, there is libgtk-x11-2.0.so.0 file.
$ rpm -qf /usr/lib64/libgtk-x11-2.0.so.0
gtk2-2.10.4-4.fc6.x86_64

So it is owned by gtk2 package. It means you can remove that dependency without
concern.

(In reply to comment #4))
> Hopefully good enough to find a sponsor :)

It does look for me that if you fix the last issues I mentioned above, this
package surely will be good enough.
Remember to make unoffical reviews of another packages to let sponsors
pay attention to you ;-)

(In reply to comment #5)
> I tried to run rpmlint for your newest package and I got:
> E: isomaster description-line-too-long ISO Master: an easy to use GUI CD image 
> editor. It allows to extract files from an ISO, add files to an ISO, and create 
> bootable ISOs - all in a graphical user interface.

I tested earlier RPM with rpmlint, but it was with a shorter description.

> Also I noticed that gtk2 dependency ought to be removed completely.
> 
> As you can see, there is libgtk-x11-2.0.so.0 file.
> $ rpm -qf /usr/lib64/libgtk-x11-2.0.so.0
> gtk2-2.10.4-4.fc6.x86_64
> 
> So it is owned by gtk2 package. It means you can remove that dependency
> without concern.

I removed it.

> It does look for me that if you fix the last issues I mentioned above, this
> package surely will be good enough.
> Remember to make unoffical reviews of another packages to let sponsors
> pay attention to you ;-)

I can try. All my comments will be with footer "I'm looking for a sponsor. Call
now: bug 220969"

Thanks for all your suggestions.


Updated version:
http://timeoff.wsisiz.edu.pl/zrzut/isomaster.spec
timeoff.wsisiz.edu.pl/zrzut/isomaster-0.6-5.src.rpm

Links with http prefix:
http://timeoff.wsisiz.edu.pl/zrzut/isomaster.spec
http://timeoff.wsisiz.edu.pl/zrzut/isomaster-0.6-5.src.rpm

* Package doesn't use our global RPM %optflags for compilation.
It uses a custom -Wall only. Makefile needs a patch to accept
$RPM_OPT_FLAGS or %{optflags}

* Desktop menu category "Application;System;" is debatable. More
appropriate would be "Application;Utility;" as it is an ordinary
application that works on files, ISO 9660 image files.

> %clean
> rm -fr %{buildroot} %{_builddir}/%{name}

Just "rm -fr %{buildroot}" is sufficient. The extracted tarball is
removed automatically after a successful build.

> #BuildRequires:	gcc-c++

The code is written in C, not C++, anyway.

(In reply to comment #8)
> * Package doesn't use our global RPM %optflags for compilation.
> It uses a custom -Wall only. Makefile needs a patch to accept
> $RPM_OPT_FLAGS or %{optflags}

I've never used that flag in my builds. I made a patch (hopefully a good one)
and I could also talk with the author about a backport changes to the upstream
version, but I don't know if that has sense, because it seems to be used only in
RPM builds and there should be something like that in a source Makefile:

ifndef OPTFLAGS
  #common defined by the author
  GLOBALFLAGS = -O2 -Wall ...
else
  GLOBALFLAGS = ${OPTFLAGS}
endif

GLOBALFLAGS += flags-speciied-for-program


What do you suggest?

> * Desktop menu category "Application;System;" is debatable. More
> appropriate would be "Application;Utility;" as it is an ordinary
> application that works on files, ISO 9660 image files.

Ok, but it's in Accessories menu now. Grip is in Sound & Video and xcdroast in
System Tools. There are all related with CD (in their own way).

> > %clean
> > rm -fr %{buildroot} %{_builddir}/%{name}
> 
> Just "rm -fr %{buildroot}" is sufficient. The extracted tarball is
> removed automatically after a successful build.

Maybe in mock. In my local, custom build directory remains. If it's not a big
problem I would prefer this option to stay (for other test builds).

> > #BuildRequires:	gcc-c++
> 
> The code is written in C, not C++, anyway.

:)
I took it from my SPEC file to other project.


Btw, project compiled with OPTFLAGS is over 10% larger than the previous one. Is
this normal?


Thanks for your sugestions.

SPEC: http://timeoff.wsisiz.edu.pl/zrzut/isomaster.spec
SRPC: http://timeoff.wsisiz.edu.pl/zrzut/isomaster-0.6-6.src.rpm

> GLOBALFLAGS += flags-speciied-for-program

That's okay. The Makefile is very simple and hardcoded.
Alternatively, it could use $(CFLAGS) in the same way that it's
possible to override it from the outside. That works with most
projects.


> %clean
>
> Maybe in mock. In my local, custom build directory remains.

rpmbuild --clean ...  

;-) Then look at the end of the build output. Btw, I prefer rpmbuild
over mock.


> Btw, project compiled with OPTFLAGS is over 10% larger
> than the previous one. Is this normal?

Increase in size is normal and due to options like
-fasynchronous-unwind-tables. Whether it's 10% or more or
less, I don't know. :)

$ rpm --eval %optflags
-O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector
--param=ssp-buffer-size=4 -m32 -march=i386 -mtune=generic
-fasynchronous-unwind-tables

Version 0.7 is out. Locale was added and few other changes in a build process
occured.

* Fri Jan 12 2007 Marcin Zajaczkowski <mszpak ATT wp DOTT pl> - 0.7-1
 - updated to 0.7
 - added locale files
 - added manual page
 - adjusted %%{optflags} patch to a new Makefile
 - added patch to correct wrong dependencies which broke parallel build
 - removed redundant deletion of builddir (--clean option in rpmbuild does the same)

Spec: timeoff.wsisiz.edu.pl/zrzut/isomaster.spec
SRPM: timeoff.wsisiz.edu.pl/zrzut/isomaster-0.7-1.src.rpm

Ehh, again with clickable links...

Spec: http://timeoff.wsisiz.edu.pl/zrzut/isomaster.spec
SRPM: http://timeoff.wsisiz.edu.pl/zrzut/isomaster-0.7-1.src.rpm

APPROVED

You seem to have no other package submitted, but I can sponsor you
nevertheless. You would continue at this step:
http://fedoraproject.org/wiki/Extras/Contributors#head-a89c07b5b8abe7748b6b39f0f89768d595234907


Packaging subtleties:

* .desktop category "Application" is deprecated, might be rejected
by a newer desktop-file-install and desktop-file-validate

* %{_mandir}/man1/isomaster.1.gz => %{_mandir}/man1/isomaster.1*
would make the spec not fail if automatic compression of manual
pages is disabled or changed

(In reply to comment #13)
> APPROVED
> 
> You seem to have no other package submitted,

I created packages for a few applications (see my webpage:
http://timeoff.wsisiz.edu.pl/rpms.html) which I recognized as useful and which
hadn't had RPMs already. Some of them (like p7zip) were already added to FE by
other people (based on my SPEC files). I made a proposal with isomaster to gain
experience with passing FE requirements and I plan to add some other packages too.

I've already submited review request of fuse-smb (bug 222742).

> but I can sponsor you nevertheless. You would continue at this step:

Thanks Michael.

>
http://fedoraproject.org/wiki/Extras/Contributors#head-a89c07b5b8abe7748b6b39f0f89768d595234907

It doesn't look to be user friendly :)

> Packaging subtleties:
> 
> * .desktop category "Application" is deprecated, might be rejected
> by a newer desktop-file-install and desktop-file-validate
> 
> * %{_mandir}/man1/isomaster.1.gz => %{_mandir}/man1/isomaster.1*
> would make the spec not fail if automatic compression of manual
> pages is disabled or changed

One question. Should I:
 - respect those suggestions in the "final" version uploaded to CVS,
 - wait with fixes until next "big" sotware version,
 - fix those things and ask once again for aproval?

> One question

The newer desktop-file-utils in Rawhide rejects .desktop files which
set Category=Application already, so you won't be able to build the
package for that target unless you fix the .desktop file. ;-)

And the thing about manual pages is fully up to you and your personal
preferences.

My question was rather about formal procedures, but I made suggested fixes anyway.

Current version (proper category and man page mapping):
http://timeoff.wsisiz.edu.pl/zrzut/isomaster.spec
http://timeoff.wsisiz.edu.pl/zrzut/isomaster-0.7-2.src.rpm


I will try to manage a Fedora Account at the weekend (maybe I will recall a
password to my GPG key earlier :) ).

I've successfully built isomaster in a devel branch. Additional branches (FC-5
and FC-6) were requested. If they are approved and built I'll mark this bug as
resolved.

Btw, can I remove FE-NEEDSPONSOR tag?

With minor problems, but a package is already available also in FC-5 and FC-6.


Thanks for your help.