Spec URL: http://hubbitus.net.ru/rpm/Fedora13/gxneur/gxneur.spec SRPM URL: http://hubbitus.net.ru/rpm/Fedora13/gxneur/gxneur-0.9.9-1.fc13.src.rpm Description: GTK front-end for X Neural Switcher. It's program like Punto Switcher, but has other functionally and features. P.S. Spec file formatted by tabs with 5 space width ( http://fedoraproject.org/wiki/PavelAlexeev/tabsize ). Please, do not start review if it is a problem for you.
New version out. http://hubbitus.net.ru/rpm/Fedora13/gxneur/gxneur-0.10.0-2.fc13.src.rpm http://hubbitus.net.ru/rpm/Fedora13/gxneur/gxneur.spec
Hi, I have a few comments on this package. Please note that I am fairly new here and so some points may be more like questions than real issues. - I start with the most unimportant one. I have the feeling that most Fedora packagers prefer one line for every single build dependency in order to make the spec file more legible. However, I did not find anything specific about this in the guidelines and so probably this is up to you. - I think you were not accurate enough with the license tags. The code in src/ states GPLv2+ (not just GPLv2) as license and some of the code in m4/ seems to be licensed under the LGPL. You find for example in the progtest.m4: * dnl Please note that the actual code of the GNU gettext library is covered * dnl by the GNU Library General Public License, and the rest of the GNU * dnl gettext package package is covered by the GNU General Public License. Maybe my understanding is not correct but if parts of this code are contained then it has to be mentioned in the License tag of the spec file. - This point may just show that I still have to learn a lot, but what is the code in m4/ good for? Are there really parts of the code of libtool or gettext included and if so, is this really allowed since Fedora has devel packages for those? (As I said, this comment may just show my lack of understanding. In that case I apologize for it and would be pleased to learn what this code is doing there.) - I think the build dependencies are not yet fully correct. I was not able to build the package without adding "GConf2-devel" as a build dependency and some are not necessary since others depend on them: * libglade2-devel requires gtk2-devel which in turn requires autoconf and automake - It seems as if you accidentally forgot to update the desktop database after installing a .desktop file: %post update-desktop-database &> /dev/null || : %postun update-desktop-database &> /dev/null || : - I think the description maybe a bit problematic. The Packaging Guidelines state that a phrasing like "a program like ..." should be avoided since people might understand you wrong (or might _want_ to understand you wrong) when it comes to trademarks. For more information see: http://fedoraproject.org/wiki/Packaging/Guidelines#Trademarks_in_Summary_or_Description Now I am not really sure if Punto Switcher is a trademark or not, but maybe it would be better just to describe what the software is doing? Maybe I am wrong here, but it's better to ask those questions _before_ getting in trouble. ;-)
(In reply to comment #2) > - I start with the most unimportant one. I have the feeling that most Fedora > packagers prefer one line for every single build dependency in order to make > the spec file more legible. However, I did not find anything specific about > this in the guidelines and so probably this is up to you. Yes, this is up to the packager and isn't a blocker here. > - I think you were not accurate enough with the license tags. The code in src/ > states GPLv2+ (not just GPLv2) as license and some of the code in m4/ seems to > be licensed under the LGPL. The License field must reflect the license of the *binary* package. Thus, only packaged files have to be considered when determining the license. The m4 files are merely required together with the autotools during configuration. So GPLv2+ is the correct tag for this package. > %post > update-desktop-database &> /dev/null || : > %postun > update-desktop-database &> /dev/null || : That's not necessary here. Calling desktop-file-install or desktop-file-validate in %install is sufficient. You usually have to refresh the desktop database only if the desktop file contains a MimeType field. > - I think the description maybe a bit problematic. The Packaging Guidelines > state that a phrasing like "a program like ..." should be avoided since people > might understand you wrong (or might _want_ to understand you wrong) when it > comes to trademarks. I agree -- a more detailed description would be nice. Without further investigation, I don't really understand what the program does.
After having a closer look at the package I would like to point out the following things: - The files TODO and README are included in the package. However, they are empty (already in the source tarball). I see no point in including these files then. Maybe you can ask upstream if this is a mistake. - If you update the version of the software you should reset the release number to 1 - As far as I can tell it is not necessary to require xneur, this dependency is found automagically during the build process. - The icon tag in the .desktop file should not have an explicit file extension (.png here) Furthermore, Martin (from comment #3) hinted me at the following points: - the autotools (autoconf, automake, autoheader, libtool, ...) are only needed when existing configure.ac or Makefile.am files are changed. This is not the case here and so they don't have to be included. - It would be better to be more precise in the %file section. Excessive globbing might lead to unwanted files being packaged. So it would be better to make the following changes: %{_bindir}/* => %{_bindir}/gxneur %{_mandir}/man?/* => %{_mandir}/man1/gxneur.1*
Thanks for the comments. License changed to GPL22+ (In reply to comment #3) > (In reply to comment #2) > > - I think the description maybe a bit problematic. The Packaging Guidelines > > state that a phrasing like "a program like ..." should be avoided since people > > might understand you wrong (or might _want_ to understand you wrong) when it > > comes to trademarks. > > I agree -- a more detailed description would be nice. Without further > investigation, I don't really understand what the program does. Ok, mention of Punto switcher removed. In summary added name of package to what frontend intended. (In reply to comment #4) > After having a closer look at the package I would like to point out the > following things: > - The files TODO and README are included in the package. However, they are > empty (already in the source tarball). I see no point in including these > files then. Maybe you can ask upstream if this is a mistake. It may be filled in further releases. > - If you update the version of the software you should reset the release number to 1 Is it required? I prefer enumerate releases through all updates. > - As far as I can tell it is not necessary to require xneur, this dependency is > found automagically during the build process. No, versioned demendency is not pulled, please see: $ rpm -qp --requires 'http://koji.fedoraproject.org/koji/getfile?taskID=2606337&name=gxneur-0.10.0-2.fc15.i686.rpm' It depend only from libxnconfig.so.10 and libxneur.so.10, but I require also exactly the same version. > - The icon tag in the .desktop file should not have an explicit file extension > (.png here) Fixed. > Furthermore, Martin (from comment #3) hinted me at the following points: > - the autotools (autoconf, automake, autoheader, libtool, ...) are only needed > when existing configure.ac or Makefile.am files are changed. This is not the > case here and so they don't have to be included. Removed. > - It would be better to be more precise in the %file section. Excessive > globbing might lead to unwanted files being packaged. So it would be better to > make the following changes: > %{_bindir}/* => %{_bindir}/gxneur > %{_mandir}/man?/* => %{_mandir}/man1/gxneur.1* Ok. Additionally add BR GConf2-devel to build in rawhide. http://hubbitus.net.ru/rpm/Fedora13/gxneur/gxneur-0.10.0-3.fc13.src.rpm http://hubbitus.net.ru/rpm/Fedora13/gxneur/gxneur.spec
(In reply to comment #5) > > - The files TODO and README are included in the package. However, they are > > empty (already in the source tarball). I see no point in including these > > files then. Maybe you can ask upstream if this is a mistake. > It may be filled in further releases. Sure, but currently, they don't contain any information, and we should not pollute the user's system with useless files. Hence, I recommend to drop them for now. > > - If you update the version of the software you should reset the release number to 1 > Is it required? I prefer enumerate releases through all updates. It's not a blocker but good practice and recommended by the guidelines: http://fedoraproject.org/wiki/Packaging/NamingGuidelines#Package_Release > It depend only from libxnconfig.so.10 and libxneur.so.10, but I require also > exactly the same version. Could you elaborate this? Why do the version numbers of gxneur and xneur have to be the same? Usually, the required xneur package should be picked based on the soname gxneur was linked against. If the library's API changes, upstream will certainly bump the soname. $ rpmlint /var/lib/mock/fedora-14-x86_64/result/*.rpm gxneur.src: W: spelling-error Summary(en_US) xneur -> aneurin, neural, neuron gxneur.src: W: spelling-error %description -l en_US xneur -> aneurin, neural, neuron gxneur.x86_64: E: zero-length /usr/share/doc/gxneur-0.10.0/TODO gxneur.x86_64: E: zero-length /usr/share/doc/gxneur-0.10.0/README 3 packages and 0 specfiles checked; 2 errors, 2 warnings.
(In reply to comment #6) > (In reply to comment #5) > > > - The files TODO and README are included in the package. However, they are > > > empty (already in the source tarball). I see no point in including these > > > files then. Maybe you can ask upstream if this is a mistake. > > It may be filled in further releases. > > Sure, but currently, they don't contain any information, and we should not > pollute the user's system with useless files. Hence, I recommend to drop them > for now. Ok, removed if you insist. > > > - If you update the version of the software you should reset the release number to 1 > > Is it required? I prefer enumerate releases through all updates. > > It's not a blocker but good practice and recommended by the guidelines: > http://fedoraproject.org/wiki/Packaging/NamingGuidelines#Package_Release I take it into account in the future, but it is not best change now and all changelog. Furthermore it break update on this particular file (at least for me). > > It depend only from libxnconfig.so.10 and libxneur.so.10, but I require also > > exactly the same version. > > Could you elaborate this? Why do the version numbers of gxneur and xneur have > to be the same? Usually, the required xneur package should be picked based on > the soname gxneur was linked against. If the library's API changes, upstream > will certainly bump the soname. May be. But I do that for ensure. Both xneur and gxneur released by same upstream developer and versions with releases synced. I do not want undefined surprises with that versions mismatch. > > $ rpmlint /var/lib/mock/fedora-14-x86_64/result/*.rpm > gxneur.src: W: spelling-error Summary(en_US) xneur -> aneurin, neural, neuron > gxneur.src: W: spelling-error %description -l en_US xneur -> aneurin, neural, > neuron > gxneur.x86_64: E: zero-length /usr/share/doc/gxneur-0.10.0/TODO > gxneur.x86_64: E: zero-length /usr/share/doc/gxneur-0.10.0/README > 3 packages and 0 specfiles checked; 2 errors, 2 warnings. xneur is name of package and application, so it is normal what spell checker does not known it. Zero-length files deleted. http://hubbitus.net.ru/rpm/Fedora13/gxneur/gxneur-0.10.0-4.fc13.src.rpm http://hubbitus.net.ru/rpm/Fedora13/gxneur/gxneur.spec
Meantime new version coming: http://hubbitus.net.ru/rpm/Fedora13/gxneur/gxneur.spec http://hubbitus.net.ru/rpm/Fedora13/gxneur/gxneur-0.11.1-1.fc13.src.rpm
New version 0.12.0: http://hubbitus.net.ru/rpm/Fedora13/gxneur/gxneur-0.12.0-1.fc13.src.rpm http://hubbitus.net.ru/rpm/Fedora13/gxneur/gxneur.spec
Martin, would you like make formal review as well all issues fixed?
Yes, sorry, I lost track of this review ticket. I'll finish the review shortly.
Hi Pavel, the package looks almost fine. Please drop Requires: xneur = %{version}. This dependency is automatically resolved by the soname of libxneur. Upstream seems to bump the soname with every new release. See also http://fedoraproject.org/wiki/PackagingDrafts/ExplicitRequires $ rpmlint ./gxneur-* gxneur.src: W: spelling-error Summary(en_US) xneur -> aneurin, neural, neuron gxneur.src: I: enchant-dictionary-not-found ru gxneur.src: W: spelling-error %description -l en_US xneur -> aneurin, neural, neuron 3 packages and 0 specfiles checked; 0 errors, 2 warnings. The above spelling errors are false positive. --------------------------------- key: [+] 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}. [X] MUST: The package must meet the Packaging Guidelines. - remove the explicit dependency on xneur. [+] MUST: The package must be licensed with a Fedora approved license. - GPLv2+ [+] 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 gxneur-0.12.0.tar.bz2* dfec2352c57cb4d60854f111ebffb350 gxneur-0.12.0.tar.bz2 dfec2352c57cb4d60854f111ebffb350 gxneur-0.12.0.tar.bz2.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: When compiling C, C++, or Fortran files, %{optflags} must be applied. [+] MUST: The spec file MUST handle locales properly. [+] MUST: If a package installs files below %{_datadir}/icons, the icon cache must be updated. [.] 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, ... [+] MUST: A package must own all directories that it creates. [+] MUST: A Fedora package must not list a file more than once in %files. [+] MUST: Permissions on files must be set properly. [.] MUST: Packages must not provide RPM dependency information when that information is not global in nature, or are otherwise handled. [.] MUST: When filtering automatically generated RPM dependency information, the filtering system implemented by Fedora must be used. [+] 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: If a package contains library files with a suffix (e.g. libfoo.so.1.1), ... [.] MUST: devel packages must require the base package using a fully versioned dependency. [+] MUST: Packages must NOT contain any .la libtool archives. [+] MUST: Packages containing GUI applications must include a %{name}.desktop file [+] MUST: .desktop files must be properly installed with desktop-file-install in the %install section. [+] MUST: Packages must not own files or directories already owned by other packages. [+] MUST: All filenames in rpm packages must be valid UTF-8. EPEL <= 5 only: [+] MUST: The spec file must contain a valid BuildRoot field. [+] MUST: At the beginning of %install, each package MUST run rm -rf %{buildroot}. [+] MUST: Each package must have a %clean section, which contains rm -rf %{buildroot}. [.] MUST: Packages containing pkgconfig(.pc) files must 'Requires: pkgconfig' [.] SHOULD: If the source package does not include license text(s) as a separate file from upstream, ... [+] SHOULD: Timestamps of files should be preserved. [+] SHOULD: The reviewer should test that the package builds in mock. [+] 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 ... [+] SHOULD: Your package should contain man pages for binaries/scripts.
Martin, thank you very much for the review! Can I make response review for you? (In reply to comment #12) > Hi Pavel, > > the package looks almost fine. Please drop Requires: xneur = %{version}. This > dependency is automatically resolved by the soname of libxneur. Upstream seems > to bump the soname with every new release. > See also http://fedoraproject.org/wiki/PackagingDrafts/ExplicitRequires > Unfortunately we can't do that! As you can see in last update of xneur it is from SVN and no soname bump was made. In last particular update I agree it only build fix and is not so important. But previous svn build (it in changelog of xneur if you want see) was to fix error what I ask upstream author. So, full version dependency should be used to pull correct version! http://hubbitus.net.ru/rpm/Fedora13/gxneur/gxneur-0.12.0-2.svn859.fc13.src.rpm http://hubbitus.net.ru/rpm/Fedora13/gxneur/gxneur.spec
(In reply to comment #13) > Martin, thank you very much for the review! Can I make response review for you? You're welcome, and thanks for the offer. However, I currently don't have any packages in the queue waiting for a reviewer. > (In reply to comment #12) > build fix and is not so important. But previous svn build (it in changelog of > xneur if you want see) was to fix error what I ask upstream author. So, full > version dependency should be used to pull correct version! If gxneur actually does require the corresponding xneur (the complete package and not just the library it was linked against), this should be mentioned somewhere in the docs. Also, the build system should check whether the proper version is present. Maybe you can ask upstream to add this. As requested by the guidelines, you should also add a comment above Requires: xneur = %{version} why this explicit dependency is necessary. http://fedoraproject.org/wiki/PackagingDrafts/ExplicitRequires ---------------- Package APPROVED ----------------
(In reply to comment #14) > If gxneur actually does require the corresponding xneur (the complete package > and not just the library it was linked against), this should be mentioned > somewhere in the docs. Good catch. I'll mention it. And yes it requires not only library! Instead it just provide GUI to change config file and then restart xneur daemon. Again thank you for the review. New Package SCM Request ======================= Package Name: gxneur Short Description: GTK front-end for X Neural Switcher Owners: hubbitus Branches: F-13 F-14 InitialCC:
Git done (by process-git-requests).
Thank you, Kevin. gxneur built in rawhide. I can't now build for Fedora 13 and 14 because there no enought version of xneur. It still in testing. So, waiting week.
gxneur-0.12.0-3.svn859.fc14 has been submitted as an update for Fedora 14. https://admin.fedoraproject.org/updates/gxneur-0.12.0-3.svn859.fc14
gxneur-0.12.0-3.svn859.fc13 has been submitted as an update for Fedora 13. https://admin.fedoraproject.org/updates/gxneur-0.12.0-3.svn859.fc13
gxneur-0.12.0-3.svn859.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 gxneur'. You can provide feedback for this update here: https://admin.fedoraproject.org/updates/gxneur-0.12.0-3.svn859.fc13
gxneur-0.12.0-3.svn859.fc14 has been pushed to the Fedora 14 stable repository. If problems still persist, please make note of it in this bug report.
gxneur-0.12.0-3.svn859.fc13 has been pushed to the Fedora 13 stable repository. If problems still persist, please make note of it in this bug report.