Bug 1086790 - Review Request: gnudos - A GNU library to help new users of the GNU system
Summary: Review Request: gnudos - A GNU library to help new users of the GNU system
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Nobody's working on this, feel free to take it
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2014-04-11 13:37 UTC by Mohammed Isam
Modified: 2014-09-25 00:15 UTC (History)
3 users (show)

Fixed In Version: gnudos-1.7-1.el7
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2014-09-19 10:01:17 UTC
psabata: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)

Description Mohammed Isam 2014-04-11 13:37:59 UTC
Spec URL: http://sites.google.com/site/mohammedisam2000/home/projects/gnudos.spec
SRPM URL: http://sites.google.com/site/mohammedisam2000/home/projects/gnudos-1.0-1.fc20.src.rpm
Description: GnuDOS package is a GNU software. It is a library designed to help new users of the GNU system, who are coming from a DOS background, fit into the picture and start using the GNU system with ease. It also addresses the console
programmers of such programs that have the look and feel of old DOS system.
The library is composed of core utilities and software applications:
* The core library (corelib) contains four utilities: Kbd (for keyboard 
handling), Screen (for screen drawing), Dialogs (for dialog boxes/window
drawing), and Strings (for strings functions).
* The software applications are three: Prime (console file manager), Mino
(console text editor), and Fog (console form designer).

Fedora Account System Username: mohammedisam

Comment 1 Michael Schwendt 2014-04-12 22:15:21 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.

Comment 2 Mohammed Isam 2014-04-13 04:51:53 UTC
(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

Comment 3 Ralf Corsepius 2014-04-13 06:57:32 UTC
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.

Comment 4 Michael Schwendt 2014-04-13 14:12:03 UTC
$ 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.

Comment 5 Mohammed Isam 2014-04-19 04:01:31 UTC
(In reply to Michael Schwendt from comment #4)
Added changelogs to the spec file and updated links..

Comment 6 Michael Schwendt 2014-04-21 23:44:52 UTC
* 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.

Comment 8 Ralf Corsepius 2014-04-26 06:37:33 UTC
* 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.

Comment 9 Ralf Corsepius 2014-04-26 06:39:01 UTC
(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"

Comment 11 Michael Schwendt 2014-05-06 21:55:21 UTC
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.

Comment 17 Petr Šabata 2014-08-18 10:52:16 UTC
I'll help Mohammed with the review.

Comment 19 Petr Šabata 2014-08-22 16:07:07 UTC
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.

Comment 20 Mohammed Isam 2014-08-23 12:22:38 UTC
(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.

Comment 22 Petr Šabata 2014-08-25 14:27:09 UTC
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.

Comment 23 Mohammed Isam 2014-08-25 20:06:42 UTC
(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.

Comment 25 Petr Šabata 2014-08-29 15:06:24 UTC
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.

Comment 26 Mohammed Isam 2014-08-29 22:43:17 UTC
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:

Comment 27 Gwyn Ciesla 2014-09-02 12:37:57 UTC
Git done (by process-git-requests).

Comment 28 Fedora Update System 2014-09-03 18:39:53 UTC
gnudos-1.7-1.fc19 has been submitted as an update for Fedora 19.
https://admin.fedoraproject.org/updates/gnudos-1.7-1.fc19

Comment 29 Fedora Update System 2014-09-03 18:40:41 UTC
gnudos-1.7-1.fc20 has been submitted as an update for Fedora 20.
https://admin.fedoraproject.org/updates/gnudos-1.7-1.fc20

Comment 30 Fedora Update System 2014-09-03 18:41:53 UTC
gnudos-1.7-1.fc21 has been submitted as an update for Fedora 21.
https://admin.fedoraproject.org/updates/gnudos-1.7-1.fc21

Comment 31 Fedora Update System 2014-09-03 18:42:36 UTC
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

Comment 32 Fedora Update System 2014-09-06 01:02:04 UTC
gnudos-1.7-1.fc21 has been pushed to the Fedora 21 testing repository.

Comment 33 Fedora Update System 2014-09-19 10:01:17 UTC
gnudos-1.7-1.fc19 has been pushed to the Fedora 19 stable repository.

Comment 34 Fedora Update System 2014-09-19 10:11:20 UTC
gnudos-1.7-1.fc20 has been pushed to the Fedora 20 stable repository.

Comment 35 Fedora Update System 2014-09-23 04:42:35 UTC
gnudos-1.7-1.fc21 has been pushed to the Fedora 21 stable repository.

Comment 36 Fedora Update System 2014-09-25 00:15:40 UTC
gnudos-1.7-1.el7 has been pushed to the Fedora EPEL 7 stable repository.


Note You need to log in before you can comment on or make changes to this bug.