Bug#794915: comments regarding your review of compute
Hi Gianfranco,
I did not expect a review so soon, many thanks!
Please find my comments inline with your original email:
On 08/08/15 06:54, Gianfranco Costamagna wrote:
Hi Ghislain,
I did a quick review of this package, and looks mostly good to me, but I have some
nitpicks (some errors) for you:
1) you seem to produce a source only library, with no binaries:
- why do you have an arch:any?
Should be arch:all indeed.
2) you seem to disable the testsuite during the build
-DBOOST_COMPUTE_BUILD_BENCHMARKS=OFF \
-DBOOST_COMPUTE_BUILD_EXAMPLES=OFF \
-DBOOST_COMPUTE_BUILD_TESTS=OFF
In my opinion you should run them, even if you do not install them, because they might be
useful to understand if your library performs well
(cfr websocketpp where I did exactly the same)
That one is a bit difficult. It is temporarily disabled because at the
time v0.4 was released the test suite was quite buggy and most tests
failed. According to the upstream git log, a few fixes have landed in
the development branch, which now makes most, but not all, tests pass
I am working with Kyle to fix the remaining failures of the test suite
for a future release.
Meanwhile, I tested the package on the test suite of the OpenCL backend
of ArrayFire (for which Boost.Compute is a dependency) and it all
passes. So, I have reasonable confidence that the Boost.Compute code
actually works and that it is worth shipping in a package today.
3) there is no need to specify the build system when there is no ambiguity
(not a problem at all, just I want to let you aware of this)
You are absolutely right. This is more coming from my "explicit better
than implicit" school of thoughts.
4) the copyright file is good, but usually is better for the Debian packaging to choose
the same license as upstream, to ease patch forwarding
Actually expat seems more permissive of the upstream BSL-1.0, so its good
Yes, Expat should be compatible with BSL-1.0, since the wording is the
same minus the added clause in the middle.
For the copyright where the license is the same you can actually merge
all the copyrights together (just list an * and the list of the copyright owners).
What Debian really care is the License, the copyright of the single file is nice to have
(so please leave it as is), but not strictly mandatory (please somebody correct me if
I'm wrong)
Files: *
Copyright: <list of all the copyright owners>
License: BSL-1.0
might have been good enough.
I usually don't take any risks and just let licensecheck do its magic here.
Since you split all of them is is for sure better, so please leave it as is :)
Plus, it took me a while ^^.
Let me know how do you want to proceed with the nitpicks, and I'll do another review.
If you're happy with my comments, I intend to:
* fix 1) now,
* fix 2) later, when the test suite is properly fixed upstream,
* let 3) and 4) as is.
And push a new version to mentors.
Please let me know how that sounds.
Best regards,
Ghislain
Reply to: