Note: This is a public test instance of Red Hat Bugzilla. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback at bugzilla.redhat.com.
Bug 220969 - Review Request: isomaster - an easy to use GUI CD image editor
Summary: Review Request: isomaster - an easy to use GUI CD image editor
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Michael Schwendt
QA Contact: Fedora Package Reviews List
URL:
Whiteboard:
Depends On:
Blocks: FE-ACCEPT
TreeView+ depends on / blocked
 
Reported: 2006-12-29 19:32 UTC by Marcin Zajaczkowski
Modified: 2009-09-21 20:33 UTC (History)
2 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2007-01-22 21:04:43 UTC
Type: ---
Embargoed:


Attachments (Terms of Use)

Description Marcin Zajaczkowski 2006-12-29 19:32:57 UTC
Spec URL: http://timeoff.wsisiz.edu.pl/zrzut/isomaster.spec
SRPM URL: http://timeoff.wsisiz.edu.pl/zrzut/isomaster-0.6-2.src.rpm
Description: 
Iso Master is an open-source, 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.

This is my first package for Fedora Extras and I'm seeking a sponsor.

Comment 1 Michał Bentkowski 2006-12-29 20:12:32 UTC
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.

Comment 2 Marcin Zajaczkowski 2006-12-29 21:44:05 UTC
(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


Comment 3 Michał Bentkowski 2006-12-29 22:14:30 UTC
(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 :)

Comment 4 Marcin Zajaczkowski 2006-12-30 11:28:46 UTC
(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


Comment 5 Michał Bentkowski 2006-12-30 16:35:56 UTC
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 ;-)


Comment 6 Marcin Zajaczkowski 2006-12-30 20:18:35 UTC
(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


Comment 8 Michael Schwendt 2007-01-08 14:21:15 UTC
* 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.


Comment 9 Marcin Zajaczkowski 2007-01-08 22:09:55 UTC
(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


Comment 10 Michael Schwendt 2007-01-12 01:45:50 UTC
> 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

Comment 11 Marcin Zajaczkowski 2007-01-13 11:56:57 UTC
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


Comment 12 Marcin Zajaczkowski 2007-01-13 11:58:13 UTC
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


Comment 13 Michael Schwendt 2007-01-14 10:18:43 UTC
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


Comment 14 Marcin Zajaczkowski 2007-01-15 22:51:40 UTC
(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?


Comment 15 Michael Schwendt 2007-01-15 23:02:58 UTC
> 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.


Comment 16 Marcin Zajaczkowski 2007-01-17 21:02:47 UTC
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 :) ).


Comment 17 Marcin Zajaczkowski 2007-01-22 20:16:16 UTC
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?


Comment 18 Marcin Zajaczkowski 2007-01-27 22:09:59 UTC
With minor problems, but a package is already available also in FC-5 and FC-6.


Thanks for your help.



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