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 1086790
Summary: | Review Request: gnudos - A GNU library to help new users of the GNU system | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Mohammed Isam <mohammed_isam1984> |
Component: | Package Review | Assignee: | Nobody's working on this, feel free to take it <nobody> |
Status: | CLOSED ERRATA | QA Contact: | Fedora Extras Quality Assurance <extras-qa> |
Severity: | medium | Docs Contact: | |
Priority: | medium | ||
Version: | rawhide | CC: | package-review, psabata, rc040203 |
Target Milestone: | --- | Flags: | psabata:
fedora-review+
gwync: fedora-cvs+ |
Target Release: | --- | ||
Hardware: | All | ||
OS: | Linux | ||
Whiteboard: | |||
Fixed In Version: | gnudos-1.7-1.el7 | Doc Type: | Bug Fix |
Doc Text: | Story Points: | --- | |
Clone Of: | Environment: | ||
Last Closed: | 2014-09-19 10:01:17 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
Mohammed Isam
2014-04-11 13:37:59 UTC
The packaging looks sloppy and half-hearted. :-( The following does not cover all issues, since there are too many, but one needs to start somewhere: > Summary: The GnuDOS library for GNU/Linux > Group: Applications/Productivity > %description > GnuDOS is a library of functions [...] > [...] > GnuDOS is a group of utilities [...] The tools included in the library > [...] The Group tag for library base packages is "System Environment/Libraries". Either that, or omit the tag, since it's optional nowadays: https://fedoraproject.org/wiki/Packaging:Guidelines#Group_tag > %files > %{_libdir}/* > %{_bindir}/* > %{_includedir}/console/* > %{_mandir}/man1/* > %{_infodir}/* > %{_docdir}/* Library and headers in a single package? That's not how it is done with Red Hat and Fedora distributions: https://fedoraproject.org/wiki/Packaging:Guidelines#Devel_Packages > %{_includedir}/console/* $ repoquery --whatprovides /usr/include/console $ https://fedoraproject.org/wiki/Packaging:Guidelines#File_and_Directory_Ownership https://fedoraproject.org/wiki/Packaging:UnownedDirectories > %{_infodir}/* https://fedoraproject.org/wiki/Packaging:ScriptletSnippets#Texinfo > $ rpmls -p gnudos-1.0-1.fc20.x86_64.rpm |grep ^d > drwxr-xr-x /usr/share/doc/fog > drwxr-xr-x /usr/share/doc/gnudos > drwxr-xr-x /usr/share/doc/mino > drwxr-xr-x /usr/share/doc/prime Not directly a blocker, but it's not nice of an RPM package called "gnudos" to occupy the doc homedirs "fog", "mino" and "prime" just because it includes executables with the same names. Typically, the package documentation is included below %_docdir/%name (i.e. %_pkgdocdir for F20 and newer). > $ rpmls -p gnudos-1.0-1.fc20.x86_64.rpm |grep lib > -rw-r--r-- /usr/lib64/libgnudos-1.0.a > -rwxr-xr-x /usr/lib64/libgnudos-1.0.la > lrwxrwxrwx /usr/lib64/libgnudos-1.0.so > lrwxrwxrwx /usr/lib64/libgnudos-1.0.so.1 > -rwxr-xr-x /usr/lib64/libgnudos-1.0.so.1.0.0 https://fedoraproject.org/wiki/Packaging:Guidelines#Packaging_Static_Libraries If you don't know the "fedora-review" tool yet, now would be a good opportunity to use it. Run "fedora-review -b 1086790" to point it at this review ticket. It evaluates the "SRPM URL:" and "Spec URL:" lines and performs many helpful checks. The tool also runs rpmlint on all packages, which you should have run before presenting this package for review. (In reply to Michael Schwendt from comment #1) > The packaging looks sloppy and half-hearted. :-( Not intended to be so :-).. Fixing > > > Summary: The GnuDOS library for GNU/Linux > > Group: Applications/Productivity Fixed > > Library and headers in a single package? That's not how it is done with Red > Hat and Fedora distributions: > > https://fedoraproject.org/wiki/Packaging:Guidelines#Devel_Packages > > > > %{_includedir}/console/* > Fixed > > https://fedoraproject.org/wiki/Packaging: > Guidelines#File_and_Directory_Ownership > https://fedoraproject.org/wiki/Packaging:UnownedDirectories > > > > %{_infodir}/* > > https://fedoraproject.org/wiki/Packaging:ScriptletSnippets#Texinfo > Added missing scriplets > > $ rpmls -p gnudos-1.0-1.fc20.x86_64.rpm |grep ^d > > drwxr-xr-x /usr/share/doc/fog > > drwxr-xr-x /usr/share/doc/gnudos > > drwxr-xr-x /usr/share/doc/mino > > drwxr-xr-x /usr/share/doc/prime > > Not directly a blocker, but it's not nice of an RPM package called "gnudos" > to occupy the doc homedirs "fog", "mino" and "prime" just because it > includes executables with the same names. Typically, the package > documentation is included below %_docdir/%name (i.e. %_pkgdocdir for F20 and > newer). > Fixed Increment the NEVR of a package each time you change something on your package. Replacing a package in-place and not even providing proper %changelogs are ruditities against reviewers, whom you are forcing to dig into files to find out if you have changed something. E.g right now your spec file's changelog doesn't reflect any of changes you might have applied, which means you are lying about the time you changed when you did change your package. $ rpmdiff gnudos-1.0-1.fc20.src.rpm.OLD1 gnudos-1.0-1.fc20.src.rpm Binary files old/gnudos-1.0.tar.gz and new/gnudos-1.0.tar.gz differ Huh? You've recreated the tarball with modified contents and without any comment. That's a rather unpolite thing to do during package review. Take your time and present packages you've self-reviewed. The review queue is not a place where to expect reviewers to do _all_ the work. Especially if you've been sponsored just recently, it can be a good idea to do your stuff painstakingly and -- in case of doubt -- ask your sponsor for a brief look at a spec file. > +%package devel > +Summary: Header files for the GnuDOS library > +Group: System Environment/Libraries The Group tag for library -devel packages is "Development/Libraries". It can be helpful to examine _existing_ packages as a reference. > +Provides: %{name}-devel-%{version} This makes no sense. There are automatic Provides for packages and subpackages. Take your time and query the built binary packages with "rpm". > +%description devel > +This package contains header files necessary to develop programs using the > GnuDOS corelib library of functions Similar to the %summary, the contents are _not_ only headers. You've misplaced the *.so symlink. It belongs in the -devel package. > > %{_includedir}/console/* > > Fixed Not. The directory is still not included in the package: > +%files devel > +%{_includedir}/console/* And in the following line you've created a new "unowned" directory: > +%{_docdir}/gnudos/* Please return to the previous comment in bugzilla and revisit the link to the File/Dir Ownership packaging guidelines. (In reply to Michael Schwendt from comment #4) Added changelogs to the spec file and updated links.. * https://fedoraproject.org/wiki/Packaging:Guidelines#Requiring_Base_Package * fedora-review fails, and I couldn't find a new src.rpm: $ fedora-review -b 1086790 INFO: Processing bugzilla bug: 1086790 INFO: Getting .spec and .srpm Urls from : 1086790 INFO: --> SRPM url: http://sites.google.com/site/mohammedisam2000/home/projects/gnudos-1.0-1.fc20.src.rpm ERROR: Cannot find usable urls here [...] If you kept the "SRPM URL:" and "Spec URL:" lines in this ticket up-to-date and accurate, you could point the fedora-review tool at this ticket. > %post -p /sbin/ldconfig > /sbin/install-info %{_infodir}/%{name}.info %{_infodir}/dir || : That won't work and will cause an error during package installation/removal. You cannot execute that scriptlet via /sbin/ldconfig. Option -p specifies the scriptlet interpreter, which is /bin/sh by default. If you change it to /sbin/ldconfig, the scriptlet body would need to be empty or executable by ldconfig. Instead, you want to execute the scriptlet via /bin/sh: %post /sbin/ldconfig /sbin/install-info %{_infodir}/%{name}.info %{_infodir}/dir || : Similarly for the %postun scriptlet. > %files > %{_libdir}/* Still half-hearted, since obviously this includes everything in %_libdir, i.e. also the files that belong only into the -devel subpackage. Spec URL: http://sites.google.com/site/mohammedisam2000/home/projects/gnudos.spec SRPM URL: http://sites.google.com/site/mohammedisam2000/home/projects/gnudos-1.1-2.fc20.src.rpm * MUSTFIX: Remove this %{_libdir}/debug/* These files are automatically packaged into *-debuginfo and not supposed to be packaged into normal packages. * MUSTFIX: Do not ship static libs %{_libdir}/libgnudos-1.0.a Shipping static libs is strongly discouraged in Fedora. If you really want to ship static libs, these must be packaged into a *-static package. As the package seems to support disabling building static libs, consider to append "--disable-static" to %configure. * MUSTFIX: Do not ship libtool-archives (*.la): %{_libdir}/libgnudos-1.0.la You need to *.la's in %install. (In reply to Ralf Corsepius from comment #8) > * MUSTFIX: Do not ship libtool-archives (*.la): > %{_libdir}/libgnudos-1.0.la > You need to *.la's in %install. Sorry, ugly typo, this should have been: "You need to remove *.la's in %install" Spec URL: http://sites.google.com/site/mohammedisam2000/home/projects/gnudos.spec SRPM URL: http://sites.google.com/site/mohammedisam2000/home/projects/gnudos-1.1-3.fc20.src.rpm Transaction check error: file /usr/share/info/dir from install of gnudos-1.1-3.fc20.x86_64 conflicts with file from package info-5.1-4.fc20.x86_64 fedora-review tells: Issues: ======= - Package installs properly. Note: Installation errors (see attachment) See: https://fedoraproject.org/wiki/Packaging:Guidelines - Texinfo files are installed using install-info in %post and %preun if package has .info files. Note: Texinfo .info file(s) in gnudos See: http://fedoraproject.org/wiki/Packaging:ScriptletSnippets#Texinfo Rpmlint ------- gnudos.x86_64: E: info-dir-file /usr/share/info/dir gnudos-devel.x86_64: W: no-dependency-on gnudos/gnudos-libs/libgnudos gnudos-devel.x86_64: W: spelling-error %description -l en_US corelib -> core lib, core-lib, scoreline gnudos-devel.x86_64: E: description-line-too-long C This package contains files necessary to develop programs using the GnuDOS corelib library of functions. The "no-dependency-on" warning refers to what has been linked at the top of comment 6 of this ticket. (In reply to Michael Schwendt from comment #11) Spec URL: http://sites.google.com/site/mohammedisam2000/home/projects/gnudos.spec SRPM URL: http://sites.google.com/site/mohammedisam2000/home/projects/gnudos-1.1-4.fc20.src.rpm Spec URL: http://sites.google.com/site/mohammedisam2000/home/projects/gnudos.spec SRPM URL: http://sites.google.com/site/mohammedisam2000/home/projects/gnudos-1.2-2.fc20.src.rpm Spec URL: http://sites.google.com/site/mohammedisam2000/home/projects/gnudos.spec SRPM URL: http://sites.google.com/site/mohammedisam2000/home/projects/gnudos-1.3-1.fc20.src.rpm Spec URL: http://sites.google.com/site/mohammedisam2000/home/projects/gnudos.spec SRPM URL: http://sites.google.com/site/mohammedisam2000/home/projects/gnudos-1.3-2.fc20.src.rpm Spec URL: http://sites.google.com/site/mohammedisam2000/home/projects/gnudos.spec SRPM URL: http://sites.google.com/site/mohammedisam2000/home/projects/gnudos-1.4-1.fc20.src.rpm I'll help Mohammed with the review. Spec URL: http://sites.google.com/site/mohammedisam2000/home/projects/gnudos.spec SRPM URL: http://sites.google.com/site/mohammedisam2000/home/projects/gnudos-1.6-1.fc20.src.rpm I find the %description a little confusing -- at first I wasn't quite sure what the difference between your "utilities" and "programs" was and kept looking for binaries which weren't there. Perhaps some rewording would be good. Update the Texinfo index before the package gets uninstalled, i.e. in a %preun section, not %postun. Also, your Texinfo files have strange permissions (755). Remove the executable bits. Next, a little subjective note -- consider making the SPEC file more readable. An empty line here and there won't hurt (e.g. separate the paragraphs in %description, separate %description from other headers, separate %changelog entries...) Packaging aside, a question: your library is called libgnudos-1.6.so* -- this will make developemt of software using your code fairly uncomfortable and I don't think this is what you want. Unless you really know why you're doing this, consider dropping the version from the actual library name. (In reply to Petr Šabata from comment #19) > I find the %description a little confusing -- at first I wasn't quite sure > what the difference between your "utilities" and "programs" was and kept > looking for binaries which weren't there. Perhaps some rewording would be > good. Fixed. Corrected this. I hope the new description is a bit clearer. Utilities are part of the shared library, and the programs are binaries that are installed along with the package. > Update the Texinfo index before the package gets uninstalled, i.e. in a > %preun section, not %postun. Fixed. That solved it, it was bugging me because rpm was always complaining on removing the package. > Also, your Texinfo files have strange permissions (755). Remove the > executable bits. Fixed. These bit permissions are set by install-info. I changed the files permission in the %files section. > Next, a little subjective note -- consider making the SPEC file more > readable. An empty line here and there won't hurt (e.g. separate the > paragraphs in %description, separate %description from other headers, > separate %changelog entries...) Fixed. Plenty of empty lines :) > Packaging aside, a question: your library is called libgnudos-1.6.so* -- > this will make developemt of software using your code fairly uncomfortable > and I don't think this is what you want. Unless you really know why you're > doing this, consider dropping the version from the actual library name. Fixed. Removed the versioning from the library. Spec URL: http://sites.google.com/site/mohammedisam2000/home/projects/gnudos.spec SRPM URL: http://sites.google.com/site/mohammedisam2000/home/projects/gnudos-1.6-3.fc20.src.rpm The tarball has changed without changing the version again. This is bad practice. The `Release' tag indicates only changes in Fedora packaging -- if you, as upstream change the code itself, you should also bump the version (as in the `Version' tag). This was already pointed out by Michael in comment #4. Your upstream changelog refers to Fedora package releases but what if other distributions want to package your software? If you feel this is just a minor update, release something like 1.6.1 and then reset the `Release' number in Fedora, so it becomes 1.6.1-1... that's just an example, the versioning scheme doesn't matter. Just realise those are two different things -- the upstream version and the Fedora package version. (In reply to Mohammed Isam from comment #20) > > Update the Texinfo index before the package gets uninstalled, i.e. in a > > %preun section, not %postun. > > Fixed. That solved it, it was bugging me because rpm was always complaining > on removing the package. Ack, handled well now. > > Also, your Texinfo files have strange permissions (755). Remove the > > executable bits. > > Fixed. These bit permissions are set by install-info. I changed the files > permission in the %files section. Nope. This was done by the `install' command (currently lines 51-54). It'd be more clear if you used the -m option instead of the %attr macro. See install(1) for details. The package is in a passable state but I'll let you do another respin just for practice. (In reply to Petr Šabata from comment #22) > The tarball has changed without changing the version again. This is bad > practice. The `Release' tag indicates only changes in Fedora packaging -- > if you, as upstream change the code itself, you should also bump the version > (as in the `Version' tag). This was already pointed out by Michael in > comment #4. > Silly mistake, the version ought to be 1.7. Sorry for that. Fixed > Nope. This was done by the `install' command (currently lines 51-54). It'd > be more clear if you used the -m option instead of the %attr macro. See > install(1) for details. > Fixed. > The package is in a passable state but I'll let you do another respin just > for practice. Updated. Thanks. Spec URL: http://sites.google.com/site/mohammedisam2000/home/projects/gnudos.spec SRPM URL: http://sites.google.com/site/mohammedisam2000/home/projects/gnudos-1.7-1.fc20.src.rpm Ok, the spec looks better now and I'm going to approve the package. However, I see you also bumped the soname while there was no need for it. This should only be done when you change the ABI so people using your library don't have to unnecessarily rebuild their software. Be sure to read about this. New Package SCM Request ======================= Package Name: gnudos Short Description: A GNU library to help new users of the GNU system Upstream URL: http://sites.google.com/site/mohammedisam2000/home/projects/ Owners: mohammedisam Branches: f19 f20 f21 el6 epel7 InitialCC: Git done (by process-git-requests). gnudos-1.7-1.fc19 has been submitted as an update for Fedora 19. https://admin.fedoraproject.org/updates/gnudos-1.7-1.fc19 gnudos-1.7-1.fc20 has been submitted as an update for Fedora 20. https://admin.fedoraproject.org/updates/gnudos-1.7-1.fc20 gnudos-1.7-1.fc21 has been submitted as an update for Fedora 21. https://admin.fedoraproject.org/updates/gnudos-1.7-1.fc21 gnudos-1.7-1.el7 has been submitted as an update for Fedora EPEL 7. https://admin.fedoraproject.org/updates/gnudos-1.7-1.el7 gnudos-1.7-1.fc21 has been pushed to the Fedora 21 testing repository. gnudos-1.7-1.fc19 has been pushed to the Fedora 19 stable repository. gnudos-1.7-1.fc20 has been pushed to the Fedora 20 stable repository. gnudos-1.7-1.fc21 has been pushed to the Fedora 21 stable repository. gnudos-1.7-1.el7 has been pushed to the Fedora EPEL 7 stable repository. |