add cnpy

[PeterMitrano]

cnpy has landed for noetic (ros-noetic-cnpy available on apt) so hopefully it's ok to add this. It depends on zlib, do I need to list that somewhere?

[Tobias-Fischer]

Hi, many thanks for your contribution! Zlib mapping is already done in https://github.com/RoboStack/ros-noetic/blob/master/conda-forge.yaml#L135 so that's all good. But cnpy specifies a static library build in https://github.com/PeterMitrano/cnpy/blob/master/CMakeLists.txt#L10 which we should avoid when building with conda. Could you add a patch that changes the ON to OFF? As soon as that's done I'm happy to merge.

[PeterMitrano]

oh, I don't think we actually need that static library option, I'll just make it shared.

[PeterMitrano]

Done PeterMitrano/cnpy@de83fe9

[traversaro]

For ensuring that the shared library works fine also in Windows, probably it make sense to just set CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS to ON (see https://stackoverflow.com/questions/54560832/cmake-windows-export-all-symbols-does-not-cover-global-variables).

[Tobias-Fischer]

Many thanks for the quick response @PeterMitrano. We now have two options: Either wait until the new versions lands in the ROS repos; or if we don't want to wait, add a new file §patch/ros-noetic-cnpy.patch` with the contents of https://github.com/PeterMitrano/cnpy/commit/de83fe97e7a96d063d565e9da7f8d25dae403a4a.patch until the patch lands in the ROS repos.

@traversaro raises a good point. We should aim for feature parity across all platforms, so it would be great if you could add cnpy to the windows and osx vinca files as well @PeterMitrano.

[PeterMitrano]

So I'm still learning how this stuff works -- does the conda stuff not get built from scratch? I had imagined that it just build every repo from source on your own build farm or something. How does it work? More specifically, do we need to wait for new debian packages to be build? I had assumed not.

I've just added the windows CMake option

[Tobias-Fischer]

Hi @PeterMitrano,
Yes, RoboStack is all a little convoluted and could definitely be streamlined ;). So we use https://github.com/RoboStack/vinca to create the recipes for conda. Within vinca, we use https://github.com/ros-infrastructure/rosdistro/tree/master/src/rosdistro to receive information about the available ROS packages. As we use rosdistro, we are bound to whatever is in the release repo of a certain package (e.g. https://github.com/ros-gbp/rviz-release for rviz). I am not sure when these distros get updated .. you might know better than I do?!?

Another option (that we currently not use) is to build packages directly from the package.xml - I haven't used this personally, @wolfv might be able to provide more info on that. But for the repo here, we rely on the released packages as described above.

So in the meantime, we can resort to using patches, which we quite often do, as upstream is sometimes slow in accepting patches and then releasing new versions of the package .. :(

Does that make sense?

[PeterMitrano]

ok but it's still building "from source" right? I actually don't know what it means to "build" a conda package, Ill have to do my homework on that. Anyways, when I do a bloom release of CNPY it will go immediately to cnpy-release, and we don't need to wait for the ROS build farm or noetic sync (which is how ros packages make it to the ros ubuntu repositories)

should I release a new version right now? I think I've fixed all the things you all suggested

[Tobias-Fischer]

Yes, it is building the last version that is released (in this case in cnpy-release) from source. So I'd say let's give it a go; if you release a new version I can merge this PR and we'll see what happens (it would be great if you add cnpy to the osx and win vinca files, too).

[PeterMitrano]

ok I've made a new release and added cnpy to the other vinca files

[wolfv]

cool. let's see what happens?

[wolfv]

worked fine on OS X but it seems that compiling the examples with MSVC did not work: https://dev.azure.com/roboforge/ros_pipelines/_build/results?buildId=1159&view=logs&j=047a1756-e6c8-53b4-34d8-03174f797694&t=f2800de1-f67b-590c-fae6-00ebf3b3dcd5&l=533

[PeterMitrano]

Ah -- adding -Wnarrowing reveals lots of terrible things. I'll look into fixing these...

[wolfv]

cool! :)

[PeterMitrano]

I've fixed the issues and made a new push to the release repo

[Tobias-Fischer]

It still found the old version (https://dev.azure.com/roboforge/ros_pipelines/_build/results?buildId=1161&view=logs&j=047a1756-e6c8-53b4-34d8-03174f797694&t=f2800de1-f67b-590c-fae6-00ebf3b3dcd5) - I'll try again later.

[traversaro]

I've fixed the issues and made a new push to the release repo

I am afraid that the new change in PeterMitrano/cnpy@f3df36b#diff-1e7de1ae2d059d21e1dd75d5812d5a34b0222cef273b7c3a2af62eb747f9d20aR10 is also not Windows friendly, as it adds GCC/Clang-style flags also on Windows/MSVC . Hopefully MSVC will just ignore the flags in the unexpected format, but I do not really remember what is its behavior in that case.

[PeterMitrano]

Good point -- I only never think about linux, bad habit. Does this work? I don't have an msvc or clang system to test on at hand

PeterMitrano/cnpy@6dbe31d

[traversaro]

By first inspection it seems that should work, but I realized later that you are using -Werror, that is never a packager friendly option as you never know which warnings future compilers will add, and so the package build could break as soon as a new compiler will come out. But that is another story. : )

[traversaro]

By first inspection it seems that should work, but I realized later that you are using -Werror, that is never a packager friendly option as you never know which warnings future compilers will add, and so the package build could break as soon as a new compiler will come out. But that is another story. : )

Longer explanation on this: https://youtu.be/_5weX5mx8hc?t=323 .

[PeterMitrano]

Thanks for the advice! I hadn't thought of that. Can we re-trigger the build? I pushed to the release repo with the fixes for other compilers

[traversaro]

Thanks @PeterMitrano ! I think @Tobias-Fischer or @wolfv know how to retrigger the build.

[Tobias-Fischer]

It's still stuck with the 0.0.3 release. I suspect we need to wait for a noetic sync. @seanyen or @wolfv will know more about the internals ..

[PeterMitrano]

Oh that would be a shame, I think those only happen once a month or so.

…

On Sun, Dec 20, 2020, 05:18 Tobias Fischer ***@***.***> wrote: It's still stuck with the 0.0.3 release. I suspect we need to wait for a noetic sync. @seanyen <https://github.com/seanyen> or @wolfv <https://github.com/wolfv> will know more about the internals .. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#18 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA6TGEWDKVE7MTKS4DD6VB3SVXFRBANCNFSM4U4UTBQA> .

[wolfv]

vinca should take the information (through rosdistro) from here: https://github.com/ros/rosdistro/blob/3bee9e37ae23bed3b62a09ae937f1bcc3bbf3803/noetic/distribution.yaml#L501-L515

Is that the latest released version, @PeterMitrano ?

If not, we might want to find ways to add packages before rosdistro formally releases them. This could also be interesting for other "third-party" stuff.

[PeterMitrano]

oh ok I didn't realize I need to do the PR to rosdistro. Unfortunately I'm on vacation now so I won't be able to do that for a few weeks. Of course I'm the only one that cares about this package being on conda so it's no rush. I'll do it when I get back.

…

[PeterMitrano]

ok I've just made a new PR to rosdistro. ros/rosdistro#27947

[PeterMitrano]

ok that PR has been merged

[Tobias-Fischer]

Seems like it needs some more work:

[1/4] Building CXX object CMakeFiles/example1.dir/example1.cpp.o
FAILED: CMakeFiles/example1.dir/example1.cpp.o 
$BUILD_PREFIX/bin/x86_64-conda-linux-gnu-c++  -I$SRC_DIR/ros-noetic-cnpy/src/work/include -I$PREFIX/include -fdiagnostics-color=always -DBOOST_ERROR_CODE_HEADER_ONLY -O3 -DNDEBUG -Wall -Wnarrowing -Werror -std=gnu++11 -MD -MT CMakeFiles/example1.dir/example1.cpp.o -MF CMakeFiles/example1.dir/example1.cpp.o.d -o CMakeFiles/example1.dir/example1.cpp.o -c $SRC_DIR/ros-noetic-cnpy/src/work/example1.cpp
$SRC_DIR/ros-noetic-cnpy/src/work/example1.cpp: In function 'int main()':
$SRC_DIR/ros-noetic-cnpy/src/work/example1.cpp:25:27: error: unused variable 'loaded_data' [-Werror=unused-variable]
   25 |     std::complex<double>* loaded_data = arr.data<std::complex<double>>();
      |                           ^~~~~~~~~~~
$SRC_DIR/ros-noetic-cnpy/src/work/example1.cpp:52:13: error: unused variable 'mv1' [-Werror=unused-variable]
   52 |     double* mv1 = arr_mv1.data<double>();
      |             ^~~
cc1plus: all warnings being treated as errors
[2/4] Building CXX object CMakeFiles/cnpy.dir/cnpy.cpp.o

As @traversaro previously mentioned, treating all warnings as errors is probably not a good idea ..

[Tobias-Fischer]

Error on Windows:

cl : Command line warning D9025 : overriding '/W3' with '/W4'
%SRC_DIR%\ros-noetic-cnpy\src\work\example1.cpp(21): error C2398: Element '1': conversion from 'const int' to '_Ty' requires a narrowing conversion
        with
        [
            _Ty=size_t
        ]
%SRC_DIR%\ros-noetic-cnpy\src\work\example1.cpp(21): error C2398: Element '2': conversion from 'const int' to '_Ty' requires a narrowing conversion
        with
        [
            _Ty=size_t
        ]
%SRC_DIR%\ros-noetic-cnpy\src\work\example1.cpp(21): error C2398: Element '3': conversion from 'const int' to '_Ty' requires a narrowing conversion
        with
        [
            _Ty=size_t
        ]
%SRC_DIR%\ros-noetic-cnpy\src\work\example1.cpp(34): error C2398: Element '1': conversion from 'const int' to '_Ty' requires a narrowing conversion
        with
        [
            _Ty=size_t
        ]
%SRC_DIR%\ros-noetic-cnpy\src\work\example1.cpp(34): error C2398: Element '2': conversion from 'const int' to '_Ty' requires a narrowing conversion
        with
        [
            _Ty=size_t
        ]
%SRC_DIR%\ros-noetic-cnpy\src\work\example1.cpp(34): error C2398: Element '3': conversion from 'const int' to '_Ty' requires a narrowing conversion
        with
        [
            _Ty=size_t
        ]
%SRC_DIR%\ros-noetic-cnpy\src\work\example1.cpp(42): error C2398: Element '1': conversion from 'const int' to '_Ty' requires a narrowing conversion
        with
        [
            _Ty=size_t
        ]
%SRC_DIR%\ros-noetic-cnpy\src\work\example1.cpp(42): error C2398: Element '2': conversion from 'const int' to '_Ty' requires a narrowing conversion
        with
        [
            _Ty=size_t
        ]
%SRC_DIR%\ros-noetic-cnpy\src\work\example1.cpp(42): error C2398: Element '3': conversion from 'const int' to '_Ty' requires a narrowing conversion
        with
        [
            _Ty=size_t
        ]
[2/4] Building CXX object CMakeFiles\cnpy.dir\cnpy.cpp.obj

[PeterMitrano]

yea I'm going to turn off Werror, although the compiler and I have no idea why it's saying those variables are unused... I can't reproduce that warning locally. I think I've fixed the problem, although I don't have a windows dev env to test on, so 🤞

Waiting for ros/rosdistro#28032

[PeterMitrano]

ok new version should be ready for testing

[traversaro]

I think I've fixed the problem, although I don't have a windows dev env to test on, so 🤞

FYI, conda can easily be used also to setup a cross-platform CI for your repo to catch early the compilation problems that you can face, for example using https://github.com/marketplace/actions/setup-miniconda .

[Tobias-Fischer]

Great, that's working fine now on Windows - the package is now available for all platforms (https://anaconda.org/RoboStack/ros-noetic-cnpy/files). I guess a rebuild of OSX/Linux is not necessary at this stage, is it? We'll rebuild all packages periodically and cnpy will then be included for all platforms.