Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
Summary: Review Request: trademgen - C++ Simulated Travel Demand Generation Library
https://bugzilla.redhat.com/show_bug.cgi?id=728815
Summary: Review Request: trademgen - C++ Simulated Travel Demand Generation Library Product: Fedora Version: rawhide Platform: All OS/Version: Linux Status: NEW Severity: medium Priority: medium Component: Package Review AssignedTo: nobody@fedoraproject.org ReportedBy: denis.arnaud_fedora@m4x.org QAContact: extras-qa@fedoraproject.org CC: notting@redhat.com, package-review@lists.fedoraproject.org Classification: Fedora Story Points: --- Type: ---
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.... 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).
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=728815
Denis Arnaud denis.arnaud_fedora@m4x.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Depends on| |702987
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=728815
Denis Arnaud denis.arnaud_fedora@m4x.org changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |dwmw2@infradead.org Component|Package Review |0xFFFF
Bug 728815 depends on bug 702987, which changed state.
Bug 702987 Summary: Review Request: stdair - C++ Standard Airline IT Library https://bugzilla.redhat.com/show_bug.cgi?id=702987
What |Old Value |New Value ---------------------------------------------------------------------------- Resolution| |ERRATA Status|ON_QA |CLOSED
--- Comment #1 from Denis Arnaud denis.arnaud_fedora@m4x.org 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... ----------------------------------------------
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=728815
Denis Arnaud denis.arnaud_fedora@m4x.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Component|0xFFFF |Package Review
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=728815
Thomas Spura tomspur@fedoraproject.org changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |tomspur@fedoraproject.org
--- Comment #2 from Thomas Spura tomspur@fedoraproject.org 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...
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=728815
--- Comment #3 from Denis Arnaud denis.arnaud_fedora@m4x.org 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... ----------------------------------------------
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.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=728815
Thomas Spura tomspur@fedoraproject.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED AssignedTo|nobody@fedoraproject.org |tomspur@fedoraproject.org Flag| |fedora-review?
--- Comment #4 from Thomas Spura tomspur@fedoraproject.org 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...)
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=728815
--- Comment #5 from Denis Arnaud denis.arnaud_fedora@m4x.org 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/tradem... 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... ----------------------------------------------
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=728815
Thomas Spura tomspur@fedoraproject.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Flag|fedora-review? |fedora-review+
--- Comment #6 from Thomas Spura tomspur@fedoraproject.org 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/tradem... 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
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=728815
Denis Arnaud denis.arnaud_fedora@m4x.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Flag| |fedora-cvs?
--- Comment #7 from Denis Arnaud denis.arnaud_fedora@m4x.org 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: --------------------------------------------------------------------------
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=728815
--- Comment #8 from Jon Ciesla limburgher@gmail.com 2011-12-14 08:02:54 EST --- Git done (by process-git-requests).
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=728815
Fedora Update System updates@fedoraproject.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |MODIFIED
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=728815
--- Comment #9 from Fedora Update System updates@fedoraproject.org 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
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=728815
--- Comment #10 from Fedora Update System updates@fedoraproject.org 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
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=728815
Fedora Update System updates@fedoraproject.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|MODIFIED |ON_QA
--- Comment #11 from Fedora Update System updates@fedoraproject.org 2011-12-16 14:48:59 EST --- trademgen-0.2.2-1.fc16 has been pushed to the Fedora 16 testing repository.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=728815
Fedora Update System updates@fedoraproject.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|ON_QA |CLOSED Fixed In Version| |trademgen-0.2.2-1.fc16 Resolution| |ERRATA Last Closed| |2011-12-24 15:50:44
--- Comment #12 from Fedora Update System updates@fedoraproject.org 2011-12-24 15:50:44 EST --- trademgen-0.2.2-1.fc16 has been pushed to the Fedora 16 stable repository.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=728815
Fedora Update System updates@fedoraproject.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Fixed In Version|trademgen-0.2.2-1.fc16 |trademgen-0.2.2-1.fc15
--- Comment #13 from Fedora Update System updates@fedoraproject.org 2011-12-24 15:52:07 EST --- trademgen-0.2.2-1.fc15 has been pushed to the Fedora 15 stable repository.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=728815
Denis Arnaud denis.arnaud_fedora@m4x.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Depends on| |781775
package-review@lists.fedoraproject.org