Bug 489233 - Review Request: rmol - C++ Revenue Management Optimisation Library (RMOL)
Review Request: rmol - C++ Revenue Management Optimisation Library (RMOL)
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
low Severity medium
: ---
: ---
Assigned To: Michael Schwendt
Fedora Extras Quality Assurance
:
Depends On: 488930 499003
Blocks:
  Show dependency treegraph
 
Reported: 2009-03-08 18:23 EDT by Denis Arnaud
Modified: 2013-01-13 06:56 EST (History)
6 users (show)

See Also:
Fixed In Version: 0.21.0-2.fc10
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-04-15 14:02:16 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
bugs.michael: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Denis Arnaud 2009-03-08 18:23:06 EDT
Spec URL: http://downloads.sourceforge.net/rmol/rmol.spec?use_mirror=ovh
SRPM URL: http://downloads.sourceforge.net/rmol/rmol-0.18.0-0.dev.1.src.rpm?use_mirror=ovh
Description: [FE-NEEDSPONSOR] That is my first package for Fedora, and I thus do need a sponsor.
------------------------------------------------
RMOL is a C++ library of Revenue Management and Optimisation classes and functions. Typically, that library may be used by service providers
(e.g., airlines offering flight seats, hotels offering rooms, rental car
companies offering rental days, broadcasting company offering advertisement 
slots, theaters offering seats, etc.) to help in optimising their revenues from
seat capacities.
Most of the algorithms implemented are public and documented in the following
book: The Theory and practice of Revenue Management, by Kalyan T. Talluri and
Garrett J. van Ryzin, Kluwer Academic Publishers, 2004, ISBN 1-4020-7701-7
Install the rmol package if you need a library for high-level
revenue management functionality.
---------------------------------------------------
Comment 1 Jason Tibbitts 2009-03-08 18:45:41 EDT
I haven't done a full review, but here are a couple of comments:

Please don't define %version and %release like that; if you have
  Version: 0.18.0
then %version is automatically defined for you to 0.18.0.

LGPL is not a valid license tag in Fedora.  Please see http://fedoraproject.org/wiki/Licensing.  I did not look at the source to see the license in use, but if it's LGPL version 2 or later, use LGPLv2+.

Please do not use the Vendor or Distribution tags.

0.dev.1 is not a valid value for the Release tag.  If the package is a released tarball with version 0.18.0, use a positive integer for the release tag with %{?dist} appended.  (Use of the dist tag is not mandatory but is recommended unless you are experienced with Fedora packaging and understand how to manage proper upgrade paths.)  If 0.18.0 has not been released and you're a using some sort of snapshot leading up to that release then other formats are allowable.  Please see http://fedoraproject.org/wiki/Packaging/NamingGuidelines for the full details.
Comment 2 Denis Arnaud 2009-03-08 19:53:47 EDT
(In reply to comment #1)
The corrected files (RPM specification file and the corresponding source RPM) are available as per the following:
Spec URL: http://downloads.sourceforge.net/rmol/rmol.spec?use_mirror=ovh
SRPM URL:
http://downloads.sourceforge.net/rmol/rmol-0.18.0-0.dev.1.src.rpm?use_mirror=ovh
Comment 3 Denis Arnaud 2009-03-08 19:59:42 EDT
(In reply to comment #1)
The corrected files (RPM specification file and the corresponding source RPM)
are available as per the following:
Spec URL: http://downloads.sourceforge.net/rmol/rmol.spec?use_mirror=ovh
SRPM URL:
http://downloads.sourceforge.net/rmol/rmol-0.18.0-1.fc10.src.rpm?use_mirror=ovh
Comment 4 Michael Schwendt 2009-03-09 07:33:48 EDT
It's likely that I've caught all issues with this package, but overall the package needs quite a lot of work:


* Run "rpmlint" on the src.rpm and built rpms. Pay extra attention to all warnings about executable files.


* The %doc file "INSTALL" is irrelevant to RPM package users.


> BuildRequires:  automake, autoconf, libtool

These are not used and therefore not required.


> %configure --enable-static 
>>>
> -rw-r--r--  /usr/lib/librmol.a
> -rwxr-xr-x  /usr/lib/librmol.la

https://fedoraproject.org/wiki/Packaging:Guidelines#Packaging_Static_Libraries


* The documentation is completely mispackaged for several reasons.

1) In the build.log:

checking for doxygen... no
[...]
  - html-doc .......... : no
[...]
  - doxygen ........... : no

2) The chosen "Group: System Environment/Libraries" for the subpackage is wrong. Should be "Group: Documentation".

3) You configure  --with-docdir=%{_docdir}/%{name}-%{version}  which conflicts with the %doc macro, because %{_docdir}/%{name}-%{version} is emptied before any local %doc files are placed in it. Display the contents of your -html-doc subpackage to see.

4) There are NamingGUidelines for documentation subpackages:
https://fedoraproject.org/wiki/PackagingNamingGuidelines#Documentation_SubPackages


* There are guidelines with examples for several types of RPM spec scriptlets. Please update your %post and %preun scriptlets:
https://fedoraproject.org/wiki/Packaging/ScriptletSnippets#Texinfo


* The -devel subpackage ought to "Requires: pkgconfig" as it places a file in %_libdir/pkgconfig/


> %{_datadir}/man/man3/%{name}.3.gz
> %{_datadir}/man/man1/%{name}-config.1.gz

Use %{_mandir} instead of %_datadir/man and '*' instead of '.gz' as the compression method chosen by rpmbuild may change.


> %{_datadir}/info/%{name}-ref.info.gz

* Use %{_infodir} instead of %{_datadir}/info and '*' instead of '.gz' as the compression method chosen by rpmbuild may change.


* Empty lines between %changelog comments increase readability. There are guidelines on how to include the package version-release in the %changelog:
https://fedoraproject.org/wiki/Packaging:Guidelines#Changelogs


* The /usr/bin/rmol-config script does questionable things:

$ rmol-config --libs
-lgsl -lgslcblas -lm -L/usr/lib -lrmol

1) Explicitly linking against libgsl* is not necessary (only for static builds!). You would need "Requires: gsl-devel" in the -devel package for this to work at all. And the pkgconfig file does not depend on gsl either, which is correct.

2) libm typically is linked implicitly by GCC.

3) -L/usr/lib is bad as /usr/lib is standard search-path already, so what this does is to override user-defined search-path with standard search-path.

$ rmol-config --cflags
-I/usr/include -I/usr/include

Same as 3) above. What this does is to override user's search-path for headers with standard search-path for headers.

$ rmol-config --cflags-debug
-I/usr/include -I/usr/include

Hmm? Not even -g is set here.


> /usr/include/rmol/config.h

This is an internal header file that is not part of the API and should not be distributed as it causes bad side-effects.
Comment 5 Denis Arnaud 2009-03-22 21:01:50 EDT
(In reply to comment #4)
Thanks a lot! I eventually have integrated all those changes, and the updated packages are as follows:
Spec URL: http://downloads.sourceforge.net/rmol/rmol.spec?use_mirror=ovh
SRPM URL:
http://downloads.sourceforge.net/rmol/rmol-0.19.0-1.fc10.src.rpm?use_mirror=ovh
Comment 6 Denis Arnaud 2009-03-22 21:32:56 EDT
(In reply to comment #5)

1. Currently, a doc package is generated for every architecture (i386, x86_64, etc.). How can we generate only one noarch RPM for the documentation? (I have not found that in the FedoraProject documentation).

2. Besides, still for the generation of the doc package, I had to copy the %{_docdir}/%{name}-%{version}/html content to %{_datadir}/%{name}-%{version}/html, as %{_docdir}/%{name}-%{version} is deleted (after the installation and) just before the content can be copied into that directory. I have looked into several other Fedora packages (dbus, xerces, cppunit), but could not find another elegant way to copy the HTML documentation into /usr/share/doc/%{name}-%{version}/html .

Thanks in advance
Comment 7 Denis Arnaud 2009-03-22 22:10:20 EDT
(In reply to comment #6)
> 
> 2. Besides, still for the generation of the doc package, I had to copy the
> %{_docdir}/%{name}-%{version}/html content to
> %{_datadir}/%{name}-%{version}/html, as %{_docdir}/%{name}-%{version} is
> deleted (after the installation and) just before the content can be copied into
> that directory. I have looked into several other Fedora packages (dbus, xerces,
> cppunit), but could not find another elegant way to copy the HTML documentation
> into /usr/share/doc/%{name}-%{version}/html .
> 

I eventually found a way to do it elegantly, and updated all the files (rmol.spec and rmol-*src.rpm) accordingly. I just added the doc/html directory to the %doc macro of the doc package (and removed all the directory copies).

So, the issue 1. of comment #6 is still open.
Comment 8 Denis Arnaud 2009-03-23 13:06:39 EDT
(In reply to comment #7)
> 
> So, the issue 1. of comment #6 is still open.  
>

That one has been corrected as well, thanks to a "BuildArch: noarch" macro in the "doc" package section. The files (rmol.spec and rmol-*.src.rpm) will be shortly updated on the SourceForge download site.

I am thus waiting for your feedback and/or approval.

Thanks in advance

Denis
Comment 9 Michael Schwendt 2009-03-24 11:02:11 EDT
* Be careful:

> -%configure --enable-static --with-docdir=%{_docdir}/%{name}-%{version}
> +#%configure --with-docdir=%{_docdir}/%{name}-%{version}
> +%configure

Some macros cannot be disabled/commented like this. Here the %configure macro is still executed twice. Safe is to replace '%' with '#'. [In %changelog, use double '%%' when referring to macro names.]


* The conflict between %doc and the installed html documentation tree still exists. "make install" copies the html tree to $RPM_BUILD_ROOT/usr/share/doc/rmol-0.19.0/ where it is deleted/overwritten with your %doc statements. To get the html tree into your -doc subpackage, you add it from the local build directory. What may seem to work here, breaks with other packages. You would lose some installed doc files silently. It's not a blocker, but one way to shoot yourself into the feet.

A common work-around [even when --with-docdir= cannot be redefined to point it to a temporary directory] is to actually use the installed documentation files rather the the local ones from the build dir. E.g.

  %define mydocs __tmp_docdir

  %install
  ...
  make install ...
  ...
  rm -rf %{mydocs} && mkdir %{mydocs}
  mv $RPM_BUILD_ROOT/%{_docdir}/%{name}-%{version}/*  %{mydocs}


  %files doc
  ...
  %doc mydocs/*
  %doc other files here

That way you can modify/fix the %{mydocs} tree after "make install" instead of modifying the extracted source tarball prior to "make install". Very convenient if you ever want to use --short-circuit builds during trouble-shooting.

[With that method, one pitfall remains, and that is related to applications which expect the documentation files in the installed %docdir. One must be careful not to move the files to a different location that doesn't match with the paths compiled into the application/program executables.]


* The following change creates an "unowned directory":

> -%{_includedir}/%{name}
> +%{_includedir}/%{name}/RMOL_Service.hpp
> +%{_includedir}/%{name}/RMOL_Types.hpp

%dir %{_includedir}/%{name}   is necessary to fix that.
https://fedoraproject.org/wiki/Packaging/UnownedDirectories


> /usr/share/aclocal/rmol.m4

I think it is acceptable not to "Requires: automake" just for this directory -- as long as the guidelines don't force packagers to do it:

| MUST: A package must own all directories that it creates. If it does
| not create a directory that it uses, then it should require a package
| which does create that directory

https://fedoraproject.org/wiki/Packaging:Guidelines#File_and_Directory_Ownership


* spectool rmol.spec 
Source0: http://download.sourceforge.net/rmol/rmol-0.19.0.tar.gz
 -> ERROR 404: Not Found.
http://downloads.sourceforge.net/rmol/rmol-0.19.0.tar.gz
 -> would work

Tarball in the src.rpm doesn't match your upstream release!


* Scratch-build:
http://koji.fedoraproject.org/koji/taskinfo?taskID=1256538
Comment 10 Denis Arnaud 2009-03-25 20:16:43 EDT
(In reply to comment #9)
> Some macros cannot be disabled/commented like this. Here the %configure macro
> is still executed twice. Safe is to replace '%' with '#'. [In %changelog, use
> double '%%' when referring to macro names.]

Thanks. I was indeed wondering why the configure script was running twice!
So, I have suppressed the commented lines.

>   %define mydocs __tmp_docdir
> 
>   %install
>   ...
>   make install ...
>   ...
>   rm -rf %{mydocs} && mkdir %{mydocs}
>   mv $RPM_BUILD_ROOT/%{_docdir}/%{name}-%{version}/html  %{mydocs}

Thanks for those explanations: things appear now clearly. I have tried to copy html files into $RPM_BUILD_ROOT%{_datadir}/%{mydocs} instead of into simply %{mydocs}, and tried the following two lines in the %files doc part:
  %doc %{_datadir}/%{mydocs}/html
  %{_datadir}/%{mydocs}/html
but the html directory was then installed (understandably!) into /usr/share/%{mydocs}, instead of into %{_docdir}/%{name}-%{version}

So, your work around works well as is.

> [With that method, one pitfall remains, and that is related to applications
> which expect the documentation files in the installed %docdir. One must be
> careful not to move the files to a different location that doesn't match with
> the paths compiled into the application/program executables.]

For that package (RMOL), it should not be a problem, as the documentation package is just developer-oriented documentation, not meant to be used by any of the RMOL applications.

> * The following change creates an "unowned directory":
> 
> > -%{_includedir}/%{name}
> > +%{_includedir}/%{name}/RMOL_Service.hpp
> > +%{_includedir}/%{name}/RMOL_Types.hpp

I have reverted to:
%{_includedir}/%{name}
(it is more elegant)

> * spectool rmol.spec 
> Source0: http://download.sourceforge.net/rmol/rmol-0.19.0.tar.gz
Fixed

> 
> Tarball in the src.rpm doesn't match your upstream release!
Fixed

In summary, all those issues/errors have been corrected.
But, a new warning has been introduced, due to the "BuildArch: noarch" directive in the "doc" package:
rpmlint rmol.spec 
rmol.spec:104: W: libdir-macro-in-noarch-package %{_libdir}/lib*.so.*
rmol.spec:111: W: libdir-macro-in-noarch-package %{_libdir}/lib%{name}.so
rmol.spec:112: W: libdir-macro-in-noarch-package %{_libdir}/pkgconfig/%{name}.pc
0 packages and 1 specfiles checked; 0 errors, 3 warnings.

It may be a bug of rpmlint, as everything else goes as expected: the RPMs are correctly generated, and they get the right content.

Awaiting for your feedback/approval.

Denis
Comment 11 Denis Arnaud 2009-03-29 06:43:46 EDT
I've tested with the new version of rpmlint (version 0.87-1, see https://bugzilla.redhat.com/show_bug.cgi?id=488930) on my Fedora 10, and the above warnings have disappeared (so, the fix of rpmlint works for me).

All the issues/errors/warnings have been corrected, and I'm waiting for your feedback/approval.

Denis
Comment 12 Michael Schwendt 2009-03-29 06:56:20 EDT
> I have tried to copy html files into $RPM_BUILD_ROOT%{_datadir}/%{mydocs} 

Yes, %doc implies a copy-operation only for relative paths. For absolute paths it's an attribute that marks files as being documentation files. To be used by rpm --query --docfiles ... and --install related --excludedocs/--includedocs options.
Comment 13 Denis Arnaud 2009-04-04 20:19:41 EDT
All the issues/errors/warnings have been corrected, and I'm waiting for your
feedback/approval.

Denis
Comment 14 Michael Schwendt 2009-04-05 12:05:47 EDT
Please post where to download the update src.rpm
Last was: rmol-0.19.0-1.fc10.src.rpm
Comment 15 Denis Arnaud 2009-04-05 12:46:36 EDT
(In reply to comment #14)
> Please post where to download the update src.rpm
> Last was: rmol-0.19.0-1.fc10.src.rpm  

It is at the same place (I have replaced the files, as they were not published anywhere else):
Spec URL: http://downloads.sourceforge.net/rmol/rmol.spec?use_mirror=ovh
SRPM URL:
http://downloads.sourceforge.net/rmol/rmol-0.19.0-1.fc10.src.rpm?use_mirror=ovh
Comment 16 Michael Schwendt 2009-04-13 06:26:05 EDT
* Looks good.

* Note though that typically one increases the "Release" value with every modification of the package (this also aids reviewers who use rpmdev-diff and similar tools). Your upstream tarball has also changed silently without increasing the minor release version. Please avoid this in the future.

* If you like to continue with the Fedora Packager sign-up procedure prior to completing the SOCI review, feel free to do so, and I'll approve your account request.

$ sha1sum rmol-0.19.0-1.fc10.src.rpm rmol-0.19.0.tar.gz
a7ccc0cfb952a63123c7b9fff34ab93ae7b4d619  rmol-0.19.0-1.fc10.src.rpm
d29bb310952168a043e104d5dd66feb68d5e1476  rmol-0.19.0.tar.gz

APPROVED
Comment 17 Denis Arnaud 2009-04-13 17:35:18 EDT
(In reply to comment #16)
> * If you like to continue with the Fedora Packager sign-up procedure prior to
> completing the SOCI review, feel free to do so, and I'll approve your account
> request.

I'll go on with the CVS package/module creation request on RMOL.

[As for SOCI, I've worked a lot on lately, and I should be able to release a new version this week]

Thanks a lot for your valuable support! I really appreciate the time and energy you have already spent on my training.
Comment 18 Denis Arnaud 2009-04-13 17:35:51 EDT
New Package CVS Request
=======================
Package Name: rmol
Short Description: C++ library of Revenue Management and Optimisation classes and functions
Owners: denisarnaud
Branches: F-9 F-10 F-11 EL-4 EL-5
InitialCC: denisarnaud
Comment 19 Kevin Fenzi 2009-04-14 12:07:39 EDT
cvs done.
Comment 20 Fedora Update System 2009-04-14 18:28:12 EDT
rmol-0.19.0-1.fc10 has been submitted as an update for Fedora 10.
http://admin.fedoraproject.org/updates/rmol-0.19.0-1.fc10
Comment 21 Fedora Update System 2009-04-15 14:02:09 EDT
rmol-0.19.0-1.fc10 has been pushed to the Fedora 10 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 22 Fedora Update System 2009-05-04 18:32:31 EDT
rmol-0.20.0-3.fc10 has been submitted as an update for Fedora 10.
http://admin.fedoraproject.org/updates/rmol-0.20.0-3.fc10
Comment 23 Fedora Update System 2009-05-04 18:33:40 EDT
rmol-0.20.0-3.fc11 has been submitted as an update for Fedora 11.
http://admin.fedoraproject.org/updates/rmol-0.20.0-3.fc11
Comment 24 Fedora Update System 2009-05-06 19:33:16 EDT
rmol-0.20.0-3.fc10 has been pushed to the Fedora 10 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 25 Fedora Update System 2009-05-09 00:19:26 EDT
rmol-0.20.0-3.fc11 has been pushed to the Fedora 11 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 26 Fedora Update System 2009-05-09 20:51:55 EDT
rmol-0.21.0-2.fc9 has been submitted as an update for Fedora 9.
http://admin.fedoraproject.org/updates/rmol-0.21.0-2.fc9
Comment 27 Fedora Update System 2009-05-09 20:52:56 EDT
rmol-0.21.0-2.fc10 has been submitted as an update for Fedora 10.
http://admin.fedoraproject.org/updates/rmol-0.21.0-2.fc10
Comment 28 Fedora Update System 2009-05-09 20:53:50 EDT
rmol-0.21.0-2.fc11 has been submitted as an update for Fedora 11.
http://admin.fedoraproject.org/updates/rmol-0.21.0-2.fc11
Comment 29 Fedora Update System 2009-05-12 00:02:40 EDT
rmol-0.21.0-2.fc9 has been pushed to the Fedora 9 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 30 Fedora Update System 2009-05-12 00:05:24 EDT
rmol-0.21.0-2.fc11 has been pushed to the Fedora 11 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 31 Fedora Update System 2009-05-12 00:12:03 EDT
rmol-0.21.0-2.fc10 has been pushed to the Fedora 10 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 32 Denis Arnaud 2009-09-26 11:14:29 EDT
Package Change Request
======================
Package Name: rmol
New Branches: F12
Owners: denisarnaud
Comment 33 Kevin Fenzi 2009-09-26 15:38:28 EDT
cvs done.

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