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
Summary: | Review Request: isomaster - an easy to use GUI CD image editor | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Marcin Zajaczkowski <mszpak> |
Component: | Package Review | Assignee: | Michael Schwendt <bugs.michael> |
Status: | CLOSED NEXTRELEASE | QA Contact: | Fedora Package Reviews List <fedora-package-review> |
Severity: | medium | Docs Contact: | |
Priority: | medium | ||
Version: | rawhide | CC: | asmith16, mr.ecik |
Target Milestone: | --- | ||
Target Release: | --- | ||
Hardware: | All | ||
OS: | Linux | ||
Whiteboard: | |||
Fixed In Version: | Doc Type: | Bug Fix | |
Doc Text: | Story Points: | --- | |
Clone Of: | Environment: | ||
Last Closed: | 2007-01-22 21:04:43 UTC | Type: | --- |
Regression: | --- | Mount Type: | --- |
Documentation: | --- | CRM: | |
Verified Versions: | Category: | --- | |
oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |
Cloudforms Team: | --- | Target Upstream Version: | |
Embargoed: | |||
Bug Depends On: | |||
Bug Blocks: | 163779 |
Description
Marcin Zajaczkowski
2006-12-29 19:32:57 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. (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. |