Bug 243607 - Review Request: gengetopt - Tool to write command line option parsing code for C programs
Review Request: gengetopt - Tool to write command line option parsing code fo...
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
low Severity medium
: ---
: ---
Assigned To: Mamoru TASAKA
Fedora Package Reviews List
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-06-10 13:19 EDT by Debarshi Ray
Modified: 2013-03-11 08:14 EDT (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-06-13 23:24:35 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
mtasaka: fedora‑review+
limburgher: fedora‑cvs+


Attachments (Terms of Use)
mock build log of gengetopt 2.19.1-2 on F-devel i386 (65.68 KB, text/plain)
2007-06-11 11:13 EDT, Mamoru TASAKA
no flags Details

  None (edit)
Description Debarshi Ray 2007-06-10 13:19:46 EDT
Spec URL: https://fedoraproject.org/wiki/DebarshiRay?action=AttachFile&do=get&target=gengetopt.spec

SRPM URL: https://fedoraproject.org/wiki/DebarshiRay?action=AttachFile&do=get&target=gengetopt-2.19.1-1.fc7.src.rpm

Description:
Gengetopt is a tool to generate C code to parse the command line arguments
argc and argv that are part of every C or C++ program. The generated code uses
the C library function getopt_long to perform the actual command line parsing.
Comment 1 Debarshi Ray 2007-06-10 13:24:14 EDT
I have submitted another review request here:
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=239901
Comment 2 Marek Mahut 2007-06-10 14:44:55 EDT
REVIEW
======

+ source files match upstream.
        b78cbb10356ce67d1d22436bce59dbdf  gengetopt-2.19.1.tar.gz
+ package meets naming and versioning guidelines.
+ specfile is properly named, is cleanly written and uses macros consistently.
+ summary is OK.
+ description is OK.
+ dist tag is present.
+ build root is OK.
+ license field matches the actual license.
+ license is open source-compatible.
+ license text included in package.
+ latest version is being packaged.
- it's recommended to fill dynamic sources like
    Source0: ftp://ftp.gnu.org/gnu/%{name}/%{name}-%{version}.tar.gz
- build of SRPM fails:

     error: Installed (but unpackaged) file(s) found:
        /usr/share/info/dir


     RPM build errors:
         Installed (but unpackaged) file(s) found:
   /usr/share/info/dir

Also don't forget to include for textinfo

     Requires(post): /sbin/install-info
     Requires(preun): /sbin/install-info

Please, correct those errors first.
Comment 3 Mamoru TASAKA 2007-06-10 15:30:39 EDT
Other points:

* Working directory on %build, %install stage
  - is actually $RPM_BUILD_DIR/%{name}-%{version}. So
    please use simply . (dot) .

* macros
  - Please check the defined macros on
    http://fedoraproject.org/wiki/Packaging/RPMMacros .
    /usr/share is already defined as %_datadir

* Header files location
  - Usually header files should be located under %_includedir, not
    under %_datadir.

* parallel make
  - parallel make failed by -j2. So please disable parallel
    make with leaving comments

* Timestamps
  - This package installs some files without any modification
    on rebuild and keeping timestamps on those files is recommended.
    For this package, it can be done by:
-----------------------------------------------------
make install DESTDIR=$RPM_BUILD_ROOT INSTALL="%{__install} -p"
-----------------------------------------------------
Comment 4 Debarshi Ray 2007-06-10 16:26:21 EDT
>  build of SRPM fails:
> ...
> [snip]
> ...
>  RPM build errors:
> ...
> [snip]
> ...
>     Requires(post): /sbin/install-info
>     Requires(preun): /sbin/install-info

Fixed.

> * Working directory on %build, %install stage

Fixed.
 
> * macros

Fixed.
 
> * Header files location

There is no object file or library installed by the upstream tarball which would
correspond to the header file. So wouldn't putting them in %{_includedir} be
wrong? I have asked upstream  about the purpose of these files, and waiting for
their reply.

> * parallel make

Fixed.
 
> * Timestamps

Fixed.

Here is the new SPEC file:
https://fedoraproject.org/wiki/DebarshiRay?action=AttachFile&do=get&target=gengetopt-2.spec
Comment 6 Toshio Kuratomi 2007-06-10 16:56:06 EDT
If I'm reading how this program works correctly, the files in
%{_datadir}/%{name}*.[ch] are templates for the files that gengetopt creates.  I
that's true then they're fine where they are.
Comment 7 Debarshi Ray 2007-06-11 05:27:43 EDT
(In reply to comment #6)
> If I'm reading how this program works correctly, the files in
> %{_datadir}/%{name}*.[ch] are templates for the files that gengetopt creates.

These files are useful when the C library does not implement the getopt_long
function. The generated parser can then optionally use these files for the
getopt_long implementation.

Please see the upstream maintainers reply here:
http://lists.gnu.org/archive/html/help-gengetopt/2007-06/msg00005.html
Comment 8 Mamoru TASAKA 2007-06-11 11:13:33 EDT
Created attachment 156723 [details]
mock build log of gengetopt 2.19.1-2 on F-devel i386

* About %{_datadir}/%{name}
  - Well, according to the explanation by upstream on the URL
    above, it may well that all files under %{_datadir}/%{name}
    be moved into main package and nuke -devel subpackage?

* make check
  - This package contains tests/ subdirectory and "make check" seems
    to do some tests. So IMO adding the following is recommended.
-----------------------------------------------------------
%check
make check
-----------------------------------------------------------
    Also, mock build says:
-----------------------------------------------------------
checking for source-highlight... no
checking for gengen... no
checking for valgrind... no
checking for help2man... no
-----------------------------------------------------------
    While I don't know about source-highlight and gengen,
    valgrind and help2man is in Fedora and with these, "make check"
    seems to do some more additional tests. So IMO you should add
    "valgrind help2man" to BuildRequires if you want to activate
    checking.
Comment 9 Debarshi Ray 2007-06-11 13:30:05 EDT
> * About %{_datadir}/%{name}
>   - Well, according to the explanation by upstream on the URL
>     above, it may well that all files under %{_datadir}/%{name}
>     be moved into main package and nuke -devel subpackage?

Done. However we may now have to live with some 'rpmlint' warnings from the
built RPM:
W: gengetopt devel-file-in-non-devel-package /usr/share/gengetopt/gnugetopt.h
W: gengetopt devel-file-in-non-devel-package /usr/share/gengetopt/getopt.c
W: gengetopt devel-file-in-non-devel-package /usr/share/gengetopt/getopt1.c

> * make check

Done.

>     Also, mock build says:
> -----------------------------------------------------------
> checking for source-highlight... no
> checking for gengen... no
> checking for valgrind... no
> checking for help2man... no

Added source-highlight, valgrind, help2man to BuildRequires. gengen does not
seem to exist as a Fedora package.

What about this:
checking for bison... no
checking for byacc... no
checking for flex... no
checking for lex... no
Comment 10 Mamoru TASAKA 2007-06-11 13:58:28 EDT
(In reply to comment #9)
> > * About %{_datadir}/%{name}
> >   - Well, according to the explanation by upstream on the URL
> >     above, it may well that all files under %{_datadir}/%{name}
> >     be moved into main package and nuke -devel subpackage?
> 
> Done. However we may now have to live with some 'rpmlint' warnings from the
> built RPM:
> W: gengetopt devel-file-in-non-devel-package /usr/share/gengetopt/gnugetopt.h
> W: gengetopt devel-file-in-non-devel-package /usr/share/gengetopt/getopt.c
> W: gengetopt devel-file-in-non-devel-package /usr/share/gengetopt/getopt1.c

Judging from the upstream comment, for this package
these messages can be ignored.

> What about this:
> checking for bison... no
> checking for byacc... no
> checking for flex... no
> checking for lex... no

Can be ignored.
Comment 11 Mamoru TASAKA 2007-06-12 02:54:45 EDT
Would you provide a new spec/srpm?
Comment 12 Debarshi Ray 2007-06-12 10:31:07 EDT
http://lists.gnu.org/archive/html/help-gengetopt/2007-06/msg00007.html

I do not think we need to include source-highlight, valgrind, help2man, etc. in
the BuildRequires.
Comment 13 Mamoru TASAKA 2007-06-12 11:21:32 EDT
What I said on comment 8 is to do "make check", not "to simply build".

Actually valgrind etc is not needed "to rebuild this package".
There are many packages (mainly related to perl module package)
which adds more BuildRequires which are not needed to rebuild the
package but is needed to check the package.
Comment 15 Mamoru TASAKA 2007-06-13 13:08:05 EDT
Okay.

----------------------------------------------------
  This package (gengetopt) is APPROVED by me
----------------------------------------------------
Comment 16 Debarshi Ray 2007-06-13 13:22:37 EDT
New Package CVS Request
=======================
Package Name: gengetopt
Short Description: Tool to write command line option parsing code for C programs.
Owners: debarshi.ray@gmail.com
Branches: FC-6, F-7
InitialCC:
Comment 17 Debarshi Ray 2007-10-04 08:16:30 EDT
Package Change Request
======================
Package Name: gengetopt
New Branches: F-8
Comment 18 Kevin Fenzi 2007-10-05 12:42:15 EDT
cvs done.
Comment 19 Andy Grimm 2013-03-09 18:40:57 EST
Package Change Request
======================
Package Name: gengetopt
New Branches: el5 el6
Owners: arg madsa gholms
InitialCC: 

Gengetopt is a requirement for full rebuilds of Eucalyptus, so we would like the maintain EPEL branches of this package.  Thanks.
Comment 20 Jon Ciesla 2013-03-11 08:14:25 EDT
Git done (by process-git-requests).

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