Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=220969
Summary: Review Request: isomaster - GUI CD image editor Product: Fedora Extras Version: devel Platform: All OS/Version: Linux Status: NEW Severity: normal Priority: normal Component: Package Review AssignedTo: nobody@fedoraproject.org ReportedBy: mszpak@wp.pl QAContact: fedora-package-review@redhat.com
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.
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: isomaster - GUI CD image editor
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=220969
mszpak@wp.pl changed:
What |Removed |Added ---------------------------------------------------------------------------- OtherBugsDependingO|163776 |177841 nThis| |
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: isomaster - GUI CD image editor
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=220969
mr.ecik@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- OtherBugsDependingO| |163776 nThis| |
------- Additional Comments From mr.ecik@gmail.com 2006-12-29 15:12 EST ------- 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.
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: isomaster - GUI CD image editor
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=220969
------- Additional Comments From mszpak@wp.pl 2006-12-29 16:44 EST ------- (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
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: isomaster - GUI CD image editor
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=220969
------- Additional Comments From mr.ecik@gmail.com 2006-12-29 17:14 EST ------- (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 :)
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: isomaster - GUI CD image editor
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=220969
------- Additional Comments From mszpak@wp.pl 2006-12-30 06:28 EST ------- (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
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: isomaster - GUI CD image editor
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=220969
------- Additional Comments From mr.ecik@gmail.com 2006-12-30 11:35 EST ------- 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 ;-)
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: isomaster - an easy to use GUI CD image editor
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=220969
mszpak@wp.pl changed:
What |Removed |Added ---------------------------------------------------------------------------- Summary|Review Request: isomaster - |Review Request: isomaster - |GUI CD image editor |an easy to use GUI CD image | |editor
------- Additional Comments From mszpak@wp.pl 2006-12-30 15:18 EST ------- (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
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: isomaster - an easy to use GUI CD image editor
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=220969
------- Additional Comments From mszpak@wp.pl 2006-12-30 15:27 EST ------- Links with http prefix: http://timeoff.wsisiz.edu.pl/zrzut/isomaster.spec http://timeoff.wsisiz.edu.pl/zrzut/isomaster-0.6-5.src.rpm
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: isomaster - an easy to use GUI CD image editor
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=220969
------- Additional Comments From bugs.michael@gmx.net 2007-01-08 09:21 EST ------- * 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.
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: isomaster - an easy to use GUI CD image editor
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=220969
------- Additional Comments From mszpak@wp.pl 2007-01-08 17:09 EST ------- (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
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: isomaster - an easy to use GUI CD image editor
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=220969
bugs.michael@gmx.net changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED AssignedTo|nobody@fedoraproject.org |bugs.michael@gmx.net OtherBugsDependingO|163776 |163778 nThis| |
------- Additional Comments From bugs.michael@gmx.net 2007-01-11 20:45 EST -------
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
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: isomaster - an easy to use GUI CD image editor
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=220969
------- Additional Comments From mszpak@wp.pl 2007-01-13 06:56 EST ------- 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
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: isomaster - an easy to use GUI CD image editor
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=220969
------- Additional Comments From mszpak@wp.pl 2007-01-13 06:58 EST ------- 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
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: isomaster - an easy to use GUI CD image editor
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=220969
bugs.michael@gmx.net changed:
What |Removed |Added ---------------------------------------------------------------------------- OtherBugsDependingO|163778 |163779 nThis| |
------- Additional Comments From bugs.michael@gmx.net 2007-01-14 05:18 EST ------- 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-a89c07b5b8abe7748b6b3...
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
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: isomaster - an easy to use GUI CD image editor
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=220969
------- Additional Comments From mszpak@wp.pl 2007-01-15 17:51 EST ------- (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-a89c07b5b8abe7748b6b3...
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?
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: isomaster - an easy to use GUI CD image editor
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=220969
------- Additional Comments From bugs.michael@gmx.net 2007-01-15 18:02 EST -------
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.
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: isomaster - an easy to use GUI CD image editor
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=220969
------- Additional Comments From mszpak@wp.pl 2007-01-17 16:02 EST ------- 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 :) ).
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: isomaster - an easy to use GUI CD image editor
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=220969
------- Additional Comments From mszpak@wp.pl 2007-01-22 15:16 EST ------- 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?
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: isomaster - an easy to use GUI CD image editor
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=220969
bugs.michael@gmx.net changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |CLOSED Resolution| |RAWHIDE OtherBugsDependingO|177841 | nThis| |
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: isomaster - an easy to use GUI CD image editor
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=220969
mszpak@wp.pl changed:
What |Removed |Added ---------------------------------------------------------------------------- Resolution|RAWHIDE |NEXTRELEASE
------- Additional Comments From mszpak@wp.pl 2007-01-27 17:09 EST ------- With minor problems, but a package is already available also in FC-5 and FC-6.
Thanks for your help.
package-review@lists.fedoraproject.org