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.
I have submitted another review request here: https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=239901
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.
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" -----------------------------------------------------
> 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
Here is the new SRPM file: https://fedoraproject.org/wiki/DebarshiRay?action=AttachFile&do=get&target=gengetopt-2.19.1-2.fc7.src.rpm
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.
(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
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.
> * 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
(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.
Would you provide a new spec/srpm?
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.
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.
Here is the SPEC file: https://fedoraproject.org/wiki/DebarshiRay?action=AttachFile&do=get&target=gengetopt-3.spec Here is the SRPM file: https://fedoraproject.org/wiki/DebarshiRay?action=AttachFile&do=get&target=gengetopt-2.19.1-3.fc7.src.rpm
Okay. ---------------------------------------------------- This package (gengetopt) is APPROVED by me ----------------------------------------------------
New Package CVS Request ======================= Package Name: gengetopt Short Description: Tool to write command line option parsing code for C programs. Owners: debarshi.ray Branches: FC-6, F-7 InitialCC:
Package Change Request ====================== Package Name: gengetopt New Branches: F-8
cvs done.
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.
Git done (by process-git-requests).