Bug 728815 - Review Request: trademgen - C++ Simulated Travel Demand Generation Library
Review Request: trademgen - C++ Simulated Travel Demand Generation Library
Status: CLOSED ERRATA
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Thomas Spura
Fedora Extras Quality Assurance
:
Depends On: 702987 781775
Blocks:
  Show dependency treegraph
 
Reported: 2011-08-07 19:08 EDT by Denis Arnaud
Modified: 2012-01-14 22:09 EST (History)
5 users (show)

See Also:
Fixed In Version: trademgen-0.2.2-1.fc15
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2011-12-24 15:50:44 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
tomspur: fedora‑review+
limburgher: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Denis Arnaud 2011-08-07 19:08:08 EDT
Spec URL: http://denisarnaud.fedorapeople.org/sim/trademgen/trademgen-0.1.0-1.spec
SRPM URL: https://downloads.sourceforge.net/project/trademgen/fedora_15/trademgen-0.1.0-1.fc15.src.rpm
Description:
aims at providing a clean API, and the corresponding C++
implementation, able to generate demand for travel solutions (e.g.,
from JFK to PEK on 25-05-2009) according to characteristics (e.g.,
Willingness-To-Pay, preferred airline, etc).
Comment 1 Denis Arnaud 2011-08-19 17:54:02 EDT
(took into account feedback from bug 728649)
----------------------------------------------
Spec URL:
http://denisarnaud.fedorapeople.org/sim/trademgen/trademgen-0.1.0-2.spec
SRPM URL:
http://denisarnaud.fedorapeople.org/sim/trademgen/trademgen-0.1.0-2.fc15.src.rpm
----------------------------------------------
Comment 2 Thomas Spura 2011-12-05 17:52:09 EST
Hmm, you might need to port this to a new soci version:
/home/tom/rpmbuild/BUILD/trademgen-0.1.0/trademgen/command/DBManager.cpp:7:28: fatal error: soci/core/soci.h: No such file or directory

$ rpm -qa | grep soci
soci-mysql-3.1.0-1.fc16.x86_64
soci-devel-3.1.0-1.fc16.x86_64
soci-mysql-devel-3.1.0-1.fc16.x86_64
soci-3.1.0-1.fc16.x86_64

yum provides points to soci-devel-3.0.0-23.fc16.x86_64.

repoquery -l soci-devel says, that there is a core library, but no soci/core/* headers. Haven't looked, if they were removed on intention or simply missing...
Comment 3 Denis Arnaud 2011-12-05 20:32:52 EST
(In reply to comment #2)
> Hmm, you might need to port this to a new soci version

In fact, I am also the maintainer of SOCI :)
Indeed, TraDemGen code had been updated to work with the new SOCI-3.1.0 version.
There were also some other small changes upstream. It gives:
----------------------------------------------
Spec URL:
http://denisarnaud.fedorapeople.org/sim/trademgen/trademgen-0.2.1-1.spec
SRPM URL:
http://denisarnaud.fedorapeople.org/sim/trademgen/trademgen-0.2.1-1.fc16.src.rpm
----------------------------------------------

rpmlint warns about missing manual page for the two small Python/Shell scripts, now also shipped within the package. Once I have got some spare time, I shall add the missing documentation. In the meantime, those scripts are kind of minimally self-documented.
Comment 4 Thomas Spura 2011-12-12 16:07:12 EST
(In reply to comment #3)
> (In reply to comment #2)
> > Hmm, you might need to port this to a new soci version
> 
> In fact, I am also the maintainer of SOCI :)

Hmm, problem solved ;)

REVIEW:
- your favorite, (ignorable in your case) rpmlint error: ;)
  $ rpmlint /home/tom/rpmbuild/RPMS/x86_64/trademgen-0.2.1-1.fc16.x86_64.rpm /home/tom/rpmbuild/RPMS/x86_64/trademgen-devel-0.2.1-1.fc16.x86_64.rpm /home/tom/rpmbuild/RPMS/x86_64/trademgen-debuginfo-0.2.1-1.fc16.x86_64.rpm /home/tom/rpmbuild/RPMS/noarch/trademgen-doc-0.2.1-1.fc16.noarch.rpm /home/tom/rpmbuild/SRPMS/trademgen-0.2.1-1.fc16.src.rpm
trademgen.src: E: invalid-spec-name
5 packages and 0 specfiles checked; 1 errors, 0 warnings.
- name ok


- doc noarch properly
- el5 needs there (buildroot & R pkgconfig)
- libraries correctly packaged
- ldconfig there
- check there
- parallel make
- macros everywhere
- %doc ok
- koji successful:
  http://koji.fedoraproject.org/koji/taskinfo?taskID=3580978


NEEDSWORK:
- The python lib doesn't seem to work or pytrademgen has a UserError
  (Maybe write an test for it? ;)):
  $ pytrademgen FRA
searchString: FRA
Raw result from the Trademgen library:
The Trademgen service has not been initialised, i.e., the init() method has not been called correctly on the Trademgener object. Please check that all the parameters are not empty and point to actual files.

- license looks weired
- It looks like Boost is bundled for running the tests:
./test/math/boost_math/empirical_distribution/count.hpp: BSL (v1.0) 
./test/math/boost_math/empirical_distribution/frequency.hpp: BSL (v1.0) 
./test/math/boost_math/empirical_distribution/ordered_sample.hpp: BSL (v1.0) 

But I can't find those files in boost-devel.
If they are not needed besides the tests, it should be ok, but grepping for it:
$ find | xargs grep functors.hpp
./test/math/boost_math/visualization/boost/svg_plot/detail/functors.hpp:// functors.hpp
./test/math/boost_math/visualization/boost/svg_plot/svg_1d_plot.hpp:#include "detail/functors.hpp"
./test/math/boost_math/visualization/boost/svg_plot/svg_boxplot.hpp:#include "detail/functors.hpp"
./test/math/boost_math/visualization/boost/svg_plot/svg_2d_plot.hpp:#include "detail/functors.hpp"
Binary file ./trademgen/libtrademgen.so.0.2.1 matches
Binary file ./trademgen/CMakeFiles/trademgenlib.dir/command/DemandManager.cpp.o matches
Binary file ./trademgen/libtrademgen.so.0.2 matches
Binary file ./trademgen/libtrademgen.so matches

and

$ locate functors.hpp
/usr/include/boost/date_time/adjust_functors.hpp
/usr/include/boost/icl/functors.hpp
/usr/include/boost/lambda/detail/lambda_functors.hpp

I'm unsure, where this dependency comes from, it just looks like the tests might be used in linking (after simply deleting tests cmake . doesn't complete...)
Comment 5 Denis Arnaud 2011-12-12 20:53:54 EST
Thanks for the review (I am conscious that it is not completed yet)!

(In reply to comment #4)
> REVIEW:
> - your favorite, (ignorable in your case) rpmlint error: ;)
> trademgen.src: E: invalid-spec-name

Indeed, you did convince me to manage versionning of the specification file with Git in GitHub: https://github.com/denisarnaud/fedorareviews/blob/trunk/sim/trademgen/trademgen_728815/trademgen.spec
In contrast, on the fedorapeople.org site, as we can not know the version of a specification without downloading it, I used to name the specification after the version, so that there is no ambiguity. For the next time, I'll use only GitHub for the specification file :)


> NEEDSWORK:
> - The python lib doesn't seem to work or pytrademgen has a UserError

Well, that Python script is just a proof-of-concept/sample, in order to demonstrate how to expose a Python API, wrapped around the original C++ library API. It does nothing very useful for now. I'll rework it later on, so that the output seems alike what is produced by the pure C++ binary. In the meantime, I believe it is still interesting to see how to cleanly wrap the C++ library within a Python script, thanks to Boost.Python.


> - license looks weired
> - It looks like Boost is bundled for running the tests:
> ./test/math/boost_math/empirical_distribution/count.hpp: BSL (v1.0) 

In fact, the test/math/ sub-directory is just a collection of samples from third party libraries. Indeed, I have benchmarked a few features of TraDemGen against some of those third party examples (that's why I left them there in the first place). For instance, a comparison of the different random generation methods/technologies can be made.

However, those third party libraries are certainly not used by TraDemGen, neither by the core library, nor by the unit tests. I have therefore just transfered them into another Git repository, more appropriate for them (https://github.com/denisarnaud/playground).

The test-related source tree has been cleaned (https://github.com/airsim/trademgen/tree/trunk/test). I have still to update the specification file to reflect that.

Following is the updated package:
----------------------------------------------
Spec URL:
http://denisarnaud.fedorapeople.org/sim/trademgen/trademgen.spec
SRPM URL:
http://denisarnaud.fedorapeople.org/sim/trademgen/trademgen-0.2.2-1.fc16.src.rpm
----------------------------------------------
Comment 6 Thomas Spura 2011-12-13 19:29:13 EST
(In reply to comment #5)
> Thanks for the review (I am conscious that it is not completed yet)!
> 
> (In reply to comment #4)
> > REVIEW:
> > - your favorite, (ignorable in your case) rpmlint error: ;)
> > trademgen.src: E: invalid-spec-name
> 
> Indeed, you did convince me to manage versionning of the specification file
> with Git in GitHub:
> https://github.com/denisarnaud/fedorareviews/blob/trunk/sim/trademgen/trademgen_728815/trademgen.spec
> In contrast, on the fedorapeople.org site, as we can not know the version of a
> specification without downloading it, I used to name the specification after
> the version, so that there is no ambiguity. For the next time, I'll use only
> GitHub for the specification file :)

Hurray ;)

> > NEEDSWORK:
> > - The python lib doesn't seem to work or pytrademgen has a UserError
> 
> Well, that Python script is just a proof-of-concept/sample, in order to
> demonstrate how to expose a Python API, wrapped around the original C++ library
> API. It does nothing very useful for now. I'll rework it later on, so that the
> output seems alike what is produced by the pure C++ binary. In the meantime, I
> believe it is still interesting to see how to cleanly wrap the C++ library
> within a Python script, thanks to Boost.Python.

Well, telling the user he should check the config, but it's failing expectantly looks a bit odd... But it's not a blocker, so ok.

> > - license looks weired
> > - It looks like Boost is bundled for running the tests:
> > ./test/math/boost_math/empirical_distribution/count.hpp: BSL (v1.0) 
> 
> In fact, the test/math/ sub-directory is just a collection of samples from
> third party libraries. Indeed, I have benchmarked a few features of TraDemGen
> against some of those third party examples (that's why I left them there in the
> first place). For instance, a comparison of the different random generation
> methods/technologies can be made.
> 
> However, those third party libraries are certainly not used by TraDemGen,
> neither by the core library, nor by the unit tests. I have therefore just
> transfered them into another Git repository, more appropriate for them
> (https://github.com/denisarnaud/playground).
> 
> The test-related source tree has been cleaned
> (https://github.com/airsim/trademgen/tree/trunk/test).

Looks fine now, thanks! :)

(In reply to comment #4)
> I can't find those files in boost-devel.
> If they are not needed besides the tests, it should be ok, but grepping for it:
> $ find | xargs grep functors.hpp
[snip]
> Binary file ./trademgen/libtrademgen.so.0.2 matches
> Binary file ./trademgen/libtrademgen.so matches
> 
> and
> 
> $ locate functors.hpp
> /usr/include/boost/date_time/adjust_functors.hpp
[snip]

The lib really uses the boost-devel adjust_functors.hpp...

#############################################################################


APPROVED
Comment 7 Denis Arnaud 2011-12-14 05:50:14 EST
Thanks for the complete review, Thomas!

(In reply to comment #6)
> The lib really uses the boost-devel adjust_functors.hpp...

Yes, sure, TraDemGen uses Boost.Date-Time, and therefore uses adjust_functors.hpp. I just stated that TraDemGen did not use/embed code from the tests I have now moved into https://github.com/denisarnaud/playground. Sorry if it was not so clear.


As for the Python script, namely pytrademgen, you are absolutely right: I've begun to re-write it and will hopefully complete it this week-end.



--------------------------------------------------------------------------
New Package SCM Request
=======================
Package Name: trademgen
Short Description: C++ Simulated Travel Demand Generation Library
Owners: denisarnaud
Branches: f15 f16 el5 el6
InitialCC:
--------------------------------------------------------------------------
Comment 8 Jon Ciesla 2011-12-14 08:02:54 EST
Git done (by process-git-requests).
Comment 9 Fedora Update System 2011-12-15 17:58:40 EST
trademgen-0.2.2-1.fc16 has been submitted as an update for Fedora 16.
https://admin.fedoraproject.org/updates/trademgen-0.2.2-1.fc16
Comment 10 Fedora Update System 2011-12-15 17:59:21 EST
trademgen-0.2.2-1.fc15 has been submitted as an update for Fedora 15.
https://admin.fedoraproject.org/updates/trademgen-0.2.2-1.fc15
Comment 11 Fedora Update System 2011-12-16 14:48:59 EST
trademgen-0.2.2-1.fc16 has been pushed to the Fedora 16 testing repository.
Comment 12 Fedora Update System 2011-12-24 15:50:44 EST
trademgen-0.2.2-1.fc16 has been pushed to the Fedora 16 stable repository.
Comment 13 Fedora Update System 2011-12-24 15:52:07 EST
trademgen-0.2.2-1.fc15 has been pushed to the Fedora 15 stable repository.

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