Bug 599097
Summary: | Review Request: libgexiv2 - Gexiv2 is a GObject-based wrapper around the Exiv2 library | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Ankur Sinha (FranciscoD) <sanjay.ankur> |
Component: | Package Review | Assignee: | Martin Gieseking <martin.gieseking> |
Status: | CLOSED ERRATA | QA Contact: | Fedora Extras Quality Assurance <extras-qa> |
Severity: | medium | Docs Contact: | |
Priority: | medium | ||
Version: | rawhide | CC: | als, fedora-package-review, jensk.maps, martin.gieseking, notting, susi.lehtola |
Target Milestone: | --- | Flags: | martin.gieseking:
fedora-review+
kevin: fedora-cvs+ |
Target Release: | --- | ||
Hardware: | All | ||
OS: | Linux | ||
Whiteboard: | |||
Fixed In Version: | libgexiv2-0.2.0-1.fc13 | Doc Type: | Bug Fix |
Doc Text: | Story Points: | --- | |
Clone Of: | Environment: | ||
Last Closed: | 2010-09-02 04:00:15 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: |
Description
Ankur Sinha (FranciscoD)
2010-06-02 17:11:40 UTC
hi, typo in description noted and changed ligexiv2 -> libgexiv2. regards, Ankur Ticket at the yorba trac regarding the configure script: http://trac.yorba.org:8000/ticket/2001 Ankur hi, updated: * Thu Jun 03 2010 Ankur Sinha <ankursinha AT fedoraproject DOT org> - 0.0.90-2 - some fixes in spec - moved *.vapi to devel - removed INSTALL from doc - added comment to yorba ticket link - corrected typo in description http://ankursinha.fedorapeople.org/libgexiv2/libgexiv2.spec http://ankursinha.fedorapeople.org/libgexiv2/libgexiv2-0.0.90-2.fc13.src.rpm mock build results at http://ankursinha.fedorapeople.org/libgexiv2/ regards, Ankur Hi, provided source rpm cannot be build on x86_64 arch due this error: error: File not found by glob: /home/als/rpmbuild/BUILDROOT libgexiv2-0.0.90-2.fc13.x86_64/usr/lib64/*.so.* You using correct %{_libdir} macro in spec file in files section, but there is hardcoded $(DESTDIR)$(PREFIX)/lib path in source Makefile in install section. Regards Als (In reply to comment #4) > Hi, > provided source rpm cannot be build on x86_64 arch due this error: > > error: File not found by glob: /home/als/rpmbuild/BUILDROOT > libgexiv2-0.0.90-2.fc13.x86_64/usr/lib64/*.so.* > > You using correct %{_libdir} macro in spec file in files section, but there is > hardcoded $(DESTDIR)$(PREFIX)/lib path in source Makefile in install section. > > Regards Als hey, I patched the Makefile spec: http://ankursinha.fedorapeople.org/libgexiv2/libgexiv2.spec srpm: http://ankursinha.fedorapeople.org/libgexiv2/libgexiv2-0.0.90-3.fc13.src.rpm rest of the mock logs : http://ankursinha.fedorapeople.org/libgexiv2/ regards, Ankur (In reply to comment #5) > srpm: > http://ankursinha.fedorapeople.org/libgexiv2/libgexiv2-0.0.90-3.fc13.src.rpm Well, this version now build correct. Good job man :) Regards Als patch sent upstream as per guidelines: http://trac.yorba.org:8000/ticket/2001#comment:7 Hi Ankur, here are some more remarks about your spec file: - add a short comment above Patch0 telling what the patch does - you can simplify calling the configure configure script as follows: echo '%{configure}' | sed '/--program-prefix=/d' >configure.new sh configure.new This also fixes the rpmlint warning about a missing libdir specification - it's not necessary to remove the empty files as they are not listed in %doc and thus not packaged - drop the %doc files from the -devel package (the files should only be added once) - %{_includedir}/* is a bit too generic. Replace it with %{_includedir}/gexiv2/ (In reply to comment #8) > - %{_includedir}/* is a bit too generic. Replace it with %{_includedir}/gexiv2/ I would say the same thing about %{_libdir}/*.so and %{_libdir}/*.so.* since I'd guess there is only one or a couple of libraries that are installed, so I usually recommend filling in these in more precision, e.g. %{_libdir}/libfoo.so and %{_libdir}/libfoo.so.* Also, I think -devel should Requires: vala for dir ownership, as there is a file put in %{_datadir}/vala/vapi/. (In reply to comment #9) > (In reply to comment #8) > > - %{_includedir}/* is a bit too generic. Replace it with %{_includedir}/gexiv2/ > > I would say the same thing about > %{_libdir}/*.so > and > %{_libdir}/*.so.* > since I'd guess there is only one or a couple of libraries that are installed, > so I usually recommend filling in these in more precision, e.g. > %{_libdir}/libfoo.so > and > %{_libdir}/libfoo.so.* > > Also, I think -devel should Requires: vala for dir ownership, as there is a > file put in %{_datadir}/vala/vapi/. (In reply to comment #8) > Hi Ankur, here are some more remarks about your spec file: > > - add a short comment above Patch0 telling what the patch does > > - you can simplify calling the configure configure script as follows: > echo '%{configure}' | sed '/--program-prefix=/d' >configure.new > sh configure.new > This also fixes the rpmlint warning about a missing libdir specification > > - it's not necessary to remove the empty files as they are not listed in %doc > and thus not packaged > > - drop the %doc files from the -devel package (the files should only be added > once) > > - %{_includedir}/* is a bit too generic. Replace it with %{_includedir}/gexiv2/ hey, I've fixed these (at least I think so) * Sat Jun 05 2010 Ankur Sinha <ankursinha AT fedoraproject DOT org> - 0.0.90-4 - changed configure portion - added Requires: vala for devel - made the file section more precise - bugzilla #599097 - changed patch to include a default LIB setting http://ankursinha.fedorapeople.org/libgexiv2/libgexiv2.spec http://ankursinha.fedorapeople.org/libgexiv2/libgexiv2-0.0.90-4.fc13.src.rpm rest of the mock build stuff at: http://ankursinha.fedorapeople.org/libgexiv2/ regards, Ankur You can also remove the variable definitions (CFLAGS, CXXFLAGS, FFLAGS) from the %build section. They are part of the %configure macro. However, since I obviously was too tired yesterday, I gave you a wrong configure replacement (sorry for that!). The Makefile doesn't recognize the locally set variables. It should work with a %build section that looks like this: echo '%{configure}' | sed '/--program-prefix=/d' >build.tmp echo 'make %{?_smp_mflags}' >>build.tmp sh build.tmp Since there are no empty files in the buildroot, you should really remove the redundant for loop. :) (In reply to comment #11) > You can also remove the variable definitions (CFLAGS, CXXFLAGS, FFLAGS) from > the %build section. They are part of the %configure macro. > However, since I obviously was too tired yesterday, I gave you a wrong > configure replacement (sorry for that!). The Makefile doesn't recognize the > locally set variables. It should work with a %build section that looks like > this: > > echo '%{configure}' | sed '/--program-prefix=/d' >build.tmp > echo 'make %{?_smp_mflags}' >>build.tmp > sh build.tmp > > > Since there are no empty files in the buildroot, you should really remove the > redundant for loop. :) hey, updated spec: http://ankursinha.fedorapeople.org/libgexiv2/libgexiv2.spec srpm: http://ankursinha.fedorapeople.org/libgexiv2/libgexiv2-0.0.90-5.fc13.src.rpm mock results: http://ankursinha.fedorapeople.org/libgexiv2/ regards, Ankur PS : I dont understand how including the make portion(the new hack) works any different from the earlier hack you had mentioned. (In reply to comment #12) > PS : I dont understand how including the make portion(the new hack) works any > different from the earlier hack you had mentioned. It has to do with the way the shell handles variables. Each shell gets its own environment so that variables defined there are usually local and unknown outside. Adding "export" makes a variable accessible in the current shell S and also in all its sub-shells, but after returning to the parent of S they are removed. So if I, for instance, define CFLAGS in a shell script "build.tmp" and use it in a spec file, CFLAGS is known inside the shell calling "build.tmp" but not in the parent (rpm) environment, i.e. all variable assignments are lost when returning to the parent (rpm) shell. Thus, the following "make" process doesn't know anything about the previously set CFLAGS variable. But when placing the make statement inside "build.tmp", the variable is still accessible. I hope that's understandable. :) A new version has been released upstream: http://yorba.org/download/gexiv2/0.0/unstable/ Would you like to update the package before the formal review? (In reply to comment #13) > (In reply to comment #12) > > PS : I dont understand how including the make portion(the new hack) works any > > different from the earlier hack you had mentioned. > > It has to do with the way the shell handles variables. Each shell gets its own > environment so that variables defined there are usually local and unknown > outside. Adding "export" makes a variable accessible in the current shell S and > also in all its sub-shells, but after returning to the parent of S they are > removed. > So if I, for instance, define CFLAGS in a shell script "build.tmp" and use it > in a spec file, CFLAGS is known inside the shell calling "build.tmp" but not in > the parent (rpm) environment, i.e. all variable assignments are lost when > returning to the parent (rpm) shell. Thus, the following "make" process doesn't > know anything about the previously set CFLAGS variable. But when placing the > make statement inside "build.tmp", the variable is still accessible. > I hope that's understandable. :) hey, thanks, that does teach me a little more :) (In reply to comment #14) > A new version has been released upstream: > http://yorba.org/download/gexiv2/0.0/unstable/ > > Would you like to update the package before the formal review? updated: http://ankursinha.fedorapeople.org/libgexiv2/libgexiv2.spec http://ankursinha.fedorapeople.org/libgexiv2/libgexiv2-0.0.91-1.fc13.src.rpm mock results at http://ankursinha.fedorapeople.org/libgexiv2/ regards, Ankur Here's the formal review. The package looks almost fine to me, except one remaining aspect: - replace %{_includedir}/gexiv2/* with %{_includedir}/gexiv2/ to make the package own the directory too (and not only the header files) $ rpmlint /var/lib/mock/fedora-13-x86_64/result/*.rpm libgexiv2.src: W: spelling-error Summary(en_US) Gexiv -> Ge xiv, Ge-xiv, Gelid libgexiv2.src: W: spelling-error %description -l en_US libgexiv -> Libreville, Liberian, Liberia libgexiv2.x86_64: W: spelling-error Summary(en_US) Gexiv -> Ge xiv, Ge-xiv, Gelid libgexiv2.x86_64: W: spelling-error %description -l en_US libgexiv -> Libreville, Liberian, Liberia libgexiv2-debuginfo.x86_64: W: spelling-error Summary(en_US) libgexiv -> Libreville, Liberian, Liberia libgexiv2-debuginfo.x86_64: W: spelling-error %description -l en_US libgexiv -> Libreville, Liberian, Liberia libgexiv2-devel.x86_64: W: spelling-error Summary(en_US) libgexiv -> Libreville, Liberian, Liberia libgexiv2-devel.x86_64: W: spelling-error %description -l en_US libgexiv -> Libreville, Liberian, Liberia libgexiv2-devel.x86_64: W: no-documentation 4 packages and 0 specfiles checked; 0 errors, 9 warnings. The above spelling errors can be ignored. --------------------------------- keys used in following checklist: [+] OK [.] OK, not applicable [X] needs work --------------------------------- [+] MUST: The package must be named according to the Package Naming Guidelines. [+] MUST: The spec file name must match the base package %{name}. [+] MUST: The package must meet the Packaging Guidelines. [+] MUST: The package must be licensed with a Fedora approved license. [+] MUST: The License field in the package spec file must match the actual license. [+] MUST: The file containing the text of the license(s) for the package must be included in %doc. [+] MUST: The spec file must be written in American English. [+] MUST: The spec file for the package MUST be legible. [+] MUST: The sources used to build the package must match the upstream source. $ md5sum libgexiv2-0.0.91.tar.gz* 16b6252efabb196ae2bf799104caa0cc libgexiv2-0.0.91.tar.gz 16b6252efabb196ae2bf799104caa0cc libgexiv2-0.0.91.tar.gz.1 [+] MUST: The package MUST successfully compile and build into binary rpms on at least one primary architecture. [.] MUST: If the package does not successfully compile, build or work on an architecture, ... [+] MUST: All build dependencies must be listed in BuildRequires. [.] MUST: The spec file MUST handle locales properly. [+] MUST: Packages storing shared library files (not just symlinks) must call ldconfig in %post and %postun. [+] MUST: Packages must NOT bundle copies of system libraries. [.] MUST: If the package is designed to be relocatable, ... [X] MUST: A package must own all directories that it creates. - directory %{_includedir}/gexiv2/ must be owned by the -devel package [+] MUST: A Fedora package must not list a file more than once in %files. [+] MUST: Permissions on files must be set properly. [+] MUST: Each package must consistently use macros. [+] MUST: The package must contain code, or permissable content. [.] MUST: Large documentation files must go in a -doc subpackage. [+] MUST: Files in %doc must not affect the runtime of the application. [+] MUST: Header files must be in a -devel package. [.] MUST: Static libraries must be in a -static package. [+] MUST: Library files that end in .so (without suffix) must go in a -devel package. [+] MUST: devel packages must require the base package using a fully versioned dependency. [+] MUST: Packages must NOT contain any .la libtool archives, these must be removed in the spec if they are built. [.] MUST: Packages containing GUI applications ... [+] MUST: Packages must not own files or directories already owned by other packages. [+] MUST: At the beginning of %install, each package MUST run rm -rf %{buildroot}. [+] MUST: All filenames in rpm packages must be valid UTF-8. [+] SHOULD: The reviewer should test that the package builds in mock. [+] SHOULD: The package should compile and build into binary rpms on all supported architectures. [+] SHOULD: The reviewer should test that the package functions as described. [+] SHOULD: If scriptlets are used, those scriptlets must be sane. [.] SHOULD: Usually, subpackages other than devel should require the base package using a fully versioned dependency. [+] SHOULD: pkgconfig(.pc) files should be placed in a -devel pkg. [+] SHOULD: If the package has file dependencies outside of /etc, /bin, /sbin, /usr/bin, or /usr/sbin consider requiring the package which provides the file instead of the file itself. (In reply to comment #16) hey, Thank you for reviewing this :) > Here's the formal review. The package looks almost fine to me, except one > remaining aspect: > - replace %{_includedir}/gexiv2/* with %{_includedir}/gexiv2/ > to make the package own the directory too (and not only the header files) > ...... > ....... > [X] MUST: A package must own all directories that it creates. > - directory %{_includedir}/gexiv2/ must be owned by the -devel package > Fixed it. new srpm : http://ankursinha.fedorapeople.org/libgexiv2/libgexiv2-0.0.91-2.fc13.src.rpm new spec http://ankursinha.fedorapeople.org/libgexiv2/libgexiv2.spec rest of mock logs at http://ankursinha.fedorapeople.org/libgexiv2/ regards, Ankur (In reply to comment #17) > Thank you for reviewing this :) You're welcome. > > [X] MUST: A package must own all directories that it creates. > > - directory %{_includedir}/gexiv2/ must be owned by the -devel package > > > Fixed it. OK, the package is ready now, and we can finish here. :) ---------------- Package APPROVED ---------------- New Package CVS Request ======================= Package Name: libgexiv2 Short Description: ligexiv2 is a GObject-based wrapper around the Exiv2 library Owners: ankursinha Branches: F-12 F-13 InitialCC: CVS done (by process-cvs-requests.py). libgexiv2-0.0.91-2.fc13 has been submitted as an update for Fedora 13. http://admin.fedoraproject.org/updates/libgexiv2-0.0.91-2.fc13 libgexiv2-0.0.91-2.fc13 has been pushed to the Fedora 13 testing repository. If problems still persist, please make note of it in this bug report. If you want to test the update, you can install it with su -c 'yum --enablerepo=updates-testing update libgexiv2'. You can provide feedback for this update here: http://admin.fedoraproject.org/updates/libgexiv2-0.0.91-2.fc13 libgexiv2-0.1.90-2.fc14 has been submitted as an update for Fedora 14. http://admin.fedoraproject.org/updates/libgexiv2-0.1.90-2.fc14 libgexiv2-0.1.90-1.fc13 has been submitted as an update for Fedora 13. http://admin.fedoraproject.org/updates/libgexiv2-0.1.90-1.fc13 libgexiv2-0.1.90-2.fc14 has been pushed to the Fedora 14 testing repository. If problems still persist, please make note of it in this bug report. If you want to test the update, you can install it with su -c 'yum --enablerepo=updates-testing update libgexiv2'. You can provide feedback for this update here: http://admin.fedoraproject.org/updates/libgexiv2-0.1.90-2.fc14 libgexiv2-0.1.90-1.fc13 has been pushed to the Fedora 13 testing repository. If problems still persist, please make note of it in this bug report. If you want to test the update, you can install it with su -c 'yum --enablerepo=updates-testing update libgexiv2'. You can provide feedback for this update here: http://admin.fedoraproject.org/updates/libgexiv2-0.1.90-1.fc13 libgexiv2-0.2.0-1.fc14 has been submitted as an update for Fedora 14. http://admin.fedoraproject.org/updates/libgexiv2-0.2.0-1.fc14 libgexiv2-0.2.0-1.fc13 has been submitted as an update for Fedora 13. http://admin.fedoraproject.org/updates/libgexiv2-0.2.0-1.fc13 libgexiv2-0.2.0-1.fc14 has been pushed to the Fedora 14 stable repository. If problems still persist, please make note of it in this bug report. libgexiv2-0.2.0-1.fc13 has been pushed to the Fedora 13 stable repository. If problems still persist, please make note of it in this bug report. |