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