[Date Prev][Date Next] [Thread Prev][Thread Next] [Date Index] [Thread Index]

Re: Packaging xsimd

Le 22/04/2021 à 19:30, Nilesh Patra a écrit :
On Thu, 22 Apr 2021 at 22:06, Julien Lamy <lamy@unistra.fr <mailto:lamy@unistra.fr>> wrote: 

    Le 22/04/2021 à 18:29, Nilesh Patra a écrit :
     >
     >
     > On Thu, 22 Apr 2021 at 21:38, Julien Lamy <lamy@unistra.fr
    <mailto:lamy@unistra.fr>
     > <mailto:lamy@unistra.fr <mailto:lamy@unistra.fr>>> wrote:
     >
     >     No, it can work with only SSE and only SSE2 enabled, which
    would match
     >     the baseline (tested with a non-AVX machine, I could not get my
     >     hands on
     >     a non-SSE2 box).
     >
     >
     >      > * Is this portable to arches other than x86 and arm?
     >
     >     Theoretically yes: there is a fallback mode which codes the SIMD
     >     instructions as loops. I have not tested it in a non-x86 and
    non-ARM
     >     environment.
     >
     >
     > I will try testing it. It looks unlikely since it seems to need a
    native
     > architecture. Likely build time tests will
     > not work, but I'll check nevertheless

    Thanks, I'll be curious of the results.


I tried in a ppc64el porter box, and I get several of:
/home/nilesh/xsimd/xsimd/test/test_batch_bool.cpp:315:1: error: 'gtest_type_params_batch_bool_test_' was not declared in this scope; did you mean 'gtest_type_params_batch_bool_test_NameGenerator'? 
   315 | TYPED_TEST(batch_bool_test, load_store)
       | ^~~~~~~~~~
/home/nilesh/xsimd/xsimd/test/test_batch_bool.cpp:315:1: error: template argument 3 is invalid 
   315 | TYPED_TEST(batch_bool_test, load_store)
       | ^~~~~~~~~~
/home/nilesh/xsimd/xsimd/test/test_batch_bool.cpp:315:1: error: 'gtest_type_params_batch_bool_test_' was not declared in this scope; did you mean 'gtest_type_params_batch_bool_test_NameGenerator'? 
   315 | TYPED_TEST(batch_bool_test, load_store)
       | ^~~~~~~~~~

And a failing build. Both for build time as well as autopkgtests.
Do you think we should for now limit arches to amd64 i386 and arm64 in d/control for now?
Since upstream only supports x86 and ARM (v7 + v8) instruction sets and since I don't have access to either mips or ppc boxes, I think restricting to the corresponding arches makes sense. I've added armhf to the ones you mention, as it should be in the supported list (I'm not familiar with ARM, let me know if it was a mistake). 


     >      > * Readme has instructions to build documentation, and you
    have added
     >      > Build Depends as well, to build it. However they are neither
     >     built nor
     >      > installed.
     >      >     If you think building and installing docs make sense,
    could
     >     you fix
     >      > it? Please install docs in a separate binary package if so.
     >
     >     Done.
     >
     >     I've also bumped the version to 7.5.0, released yesterday
    while I was
     >     packaging :)
     >
     >
     > I do not see your changes on salsa[1] - did you forget to push in
    any case?

    Yes :( Fixed now.


I have following comments to make:
* Why is the package named xsimd-dev instead of libxsimd-dev? It might match xtensor, but AFAICS that's against the library style packaging. For ref: https://www.netfort.gr.jp/~dancer/column/libpkg-guide/libpkg-guide.html 


<https://www.netfort.gr.jp/~dancer/column/libpkg-guide/libpkg-guide.html>
It was indeed chosen to match xtensor/xtl. I've reverted it to match the usual naming (libxsimd-dev / libxsimd-doc). 


* Please commit v7.5.0 to pristine-tar


Done.
* Some files in ./include have excerpts from code that belongs to Boost 


Software license. For example: ./include/xsimd/math/xsimd_error.hpp
    This should be mentioned in d/cpopyright
   Also, this file: ./test/test_constant_batch.cpp has a different copyright holder "Serge Guelton and QuantStack" so this should also be mentioned explicitly with 
    Files: ./test/test_constant_batch.cpp
    Copyright: Serge Guelton and QuantStack
    ..............
   This is not exhaustive, please consider doing a scrutiny for the entire codebase and mention copyrights for any files that differ/have different license 

Done, licensecheck + manual check for the double- and triple-licensed files.
* Autopkgtest on salsa CI fails, consider fixing it: https://salsa.debian.org/science-team/xsimd/-/jobs/1599080 <https://salsa.debian.org/science-team/xsimd/-/jobs/1599080> 

Done, changed to a simpler set-up than re-running unit tests.

--
Julien

Attachment: OpenPGP_signature
Description: OpenPGP digital signature

Reply to: