Skip to content

Conversation

@sloretz
Copy link
Contributor

@sloretz sloretz commented Jul 31, 2024

The ROS Noetic armhf build for eigenpy is failing - likely caused by #455, and it's causing regressions blocking the Noetic Sync. May I request a release to Noetic when this PR or another fix for the issue is merged?

This fixes the issue by using SFINAE and partial template specialization(as suggested here) to remove the specializations of NumpyEquivalentType<long long> and NumpyEquivalentType<unsigned long long> when they're the same as int64_t and uint64_t respectively.

https://build.ros.org/view/Nbin_ufhf_uFhf/job/Nbin_ufhf_uFhf__eigenpy__ubuntu_focal_armhf__binary/96

23:06:42 /usr/lib/ccache/c++  -Deigenpy_EXPORTS -I/tmp/binarydeb/ros-noetic-eigenpy-3.7.0/.obj-arm-linux-gnueabihf -I/tmp/binarydeb/ros-noetic-eigenpy-3.7.0/include -isystem /tmp/binarydeb/ros-noetic-eigenpy-3.7.0/.obj-arm-linux-gnueabihf/include -isystem /usr/include/eigen3 -isystem /usr/lib/python3/dist-packages/numpy/core/include -isystem /usr/include/python3.8  -Wno-long-long -Wall -Wextra -Wcast-align -Wcast-qual -Wformat -Wwrite-strings -Wconversion -g -O2 -fdebug-prefix-map=/tmp/binarydeb/ros-noetic-eigenpy-3.7.0=. -fstack-protector-strong -Wformat -Werror=format-security -DNDEBUG -Wdate-time -D_FORTIFY_SOURCE=2 -fPIC   -Wno-conversion -std=gnu++14 -o CMakeFiles/eigenpy.dir/src/solvers/preconditioners.cpp.o -c /tmp/binarydeb/ros-noetic-eigenpy-3.7.0/src/solvers/preconditioners.cpp
23:06:50 In file included from /tmp/binarydeb/ros-noetic-eigenpy-3.7.0/include/eigenpy/fwd.hpp:87,
23:06:50                  from /tmp/binarydeb/ros-noetic-eigenpy-3.7.0/include/eigenpy/solvers/BasicPreconditioners.hpp:11,
23:06:50                  from /tmp/binarydeb/ros-noetic-eigenpy-3.7.0/src/solvers/preconditioners.cpp:8:
23:06:50 /tmp/binarydeb/ros-noetic-eigenpy-3.7.0/include/eigenpy/numpy.hpp:143:8: error: redefinition of ‘struct eigenpy::NumpyEquivalentType<long long int>’
23:06:50   143 | struct NumpyEquivalentType<long long> {
23:06:50       |        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
23:06:50 /tmp/binarydeb/ros-noetic-eigenpy-3.7.0/include/eigenpy/numpy.hpp:115:8: note: previous definition of ‘struct eigenpy::NumpyEquivalentType<long long int>’
23:06:50   115 | struct NumpyEquivalentType<int64_t> {
23:06:50       |        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
23:06:50 /tmp/binarydeb/ros-noetic-eigenpy-3.7.0/include/eigenpy/numpy.hpp:147:8: error: redefinition of ‘struct eigenpy::NumpyEquivalentType<long long unsigned int>’
23:06:50   147 | struct NumpyEquivalentType<unsigned long long> {
23:06:50       |        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
23:06:50 /tmp/binarydeb/ros-noetic-eigenpy-3.7.0/include/eigenpy/numpy.hpp:119:8: note: previous definition of ‘struct eigenpy::NumpyEquivalentType<long long unsigned int>’
23:06:50   119 | struct NumpyEquivalentType<uint64_t> {
23:06:50       |        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
23:06:50 make[4]: *** [CMakeFiles/eigenpy.dir/build.make:66: CMakeFiles/eigenpy.dir/src/solvers/preconditioners.cpp.o] Error 1

I tested that eigenpy builds with this PR on an amd64 machine, but that of course won't show the build failure. Instead I made sure the method for the fix works using godbolt.org with the 32bit ARM GCC 9.3.0 compiler option with the code below:

#include <cstdint>
#include <type_traits>

template<typename S, typename Enable = void>
struct FoobarType{};

template<>
struct FoobarType<int64_t>{};

template<typename S>
struct FoobarType<S, 
std::enable_if_t<
!std::is_same<int64_t, long long>::value &&
std::is_same<S, long long>::value>>{};

template<>
struct FoobarType<uint64_t>{};

template<typename S>
struct FoobarType<S, 
std::enable_if_t<
!std::is_same<uint64_t, unsigned long long>::value &&
std::is_same<S, unsigned long long>::value>>{};

int main() {
    FoobarType<long long> bar;
    FoobarType<int64_t> baz;

    FoobarType<unsigned long long> ubar;
    FoobarType<uint64_t> ubaz;
}

@sloretz
Copy link
Contributor Author

sloretz commented Aug 7, 2024

@wxmerkt Mind reviewing this one? This bug is causing regressions in Noetic on armhf, and that's blocking the Noetic sync.

wxmerkt
wxmerkt previously approved these changes Aug 7, 2024
Copy link
Member

@wxmerkt wxmerkt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for putting this together and the ping. Looks good - let's wait for CI to pass

wxmerkt
wxmerkt previously approved these changes Aug 7, 2024
Copy link
Member

@wxmerkt wxmerkt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good from my end. @jcarpent is this good to merge?

@jcarpent
Copy link
Contributor

Thanks a lot, @sloretz, for providing this fix, and thanks a lot, @wxmerkt, for reviewing this PR carefully.
I will provide a new release soon.

@jcarpent jcarpent merged commit e93e1a7 into stack-of-tasks:devel Aug 13, 2024
@sloretz sloretz deleted the sfinae_NumpyEquivalentType branch August 14, 2024 18:18
nim65s added a commit to nim65s/robotpkg that referenced this pull request Oct 8, 2024
    ## [3.8.2] - 2024-08-26

    ### Fixed
    - Fix function signature on Windows (stack-of-tasks/eigenpy#494)

    ## [3.8.1] - 2024-08-25

    ### Fixed
    - Fix compatibility issue with NumPy 2.x on Windows (stack-of-tasks/eigenpy#492)

    ## [3.8.0] - 2024-08-14

    ### Added
    - Add compatibility with jrl-cmakemodules workspace (stack-of-tasks/eigenpy#485)
    - Remove support of Python 3.7 (stack-of-tasks/eigenpy#490)

    ### Fixed
    - Remove CMake CMP0167 warnings (stack-of-tasks/eigenpy#487)
    - Fix compilation error on armhf (stack-of-tasks/eigenpy#488)
nim65s added a commit to nim65s/robotpkg that referenced this pull request Oct 9, 2024
    ## [3.10.0] - 2024-09-26

    ### Added

    - `GenericMapPythonVisitor`/`StdMapPythonVisitor` can now take an extra visitor argument in the `expose()` method, similar to `StdVectorPythonVisitor`

    ### Changed

    - Move `GenericMapPythonVisitor` to its own header `eigenpy/map.hpp`
    - Rename `overload_base_get_item_for_std_map` to `overload_base_get_item_for_map`, move out of `eigenpy::details` namespace
    - Move `EmptyPythonVisitor` to new header `eigenpy/utils/empty-visitor.hpp`

    ## [3.9.1] - 2024-09-19

    ### Added

    - Add test returning reference of std::pair stack-of-tasks/eigenpy#503
    - Add more general visitor `GenericMapPythonVisitor` for map types test `boost::unordered_map<std::string, int>` stack-of-tasks/eigenpy#504
    - Support for non-[default-contructible](https://en.cppreference.com/w/cpp/named_req/DefaultConstructible) types in map types stack-of-tasks/eigenpy#504
    - Add type_info helpers stack-of-tasks/eigenpy#502
    - Add NumPy 2 support stack-of-tasks/eigenpy#496

    ### Changed

    - Move `StdMapPythonVisitor` out of `eigenpy::python` namespace, which was a mistake stack-of-tasks/eigenpy#504

    ## [3.9.0] - 2024-08-31

    ### Changed
    - The `exposeStdVectorEigenSpecificType()` template function now takes the vector allocator as a template parameter stack-of-tasks/eigenpy#500

    ### Added
    - Add bp::dist to std::map converter stack-of-tasks/eigenpy#499

    ## [3.8.2] - 2024-08-26

    ### Fixed
    - Fix function signature on Windows stack-of-tasks/eigenpy#494

    ## [3.8.1] - 2024-08-25

    ### Fixed
    - Fix compatibility issue with NumPy 2.x on Windows stack-of-tasks/eigenpy#492

    ## [3.8.0] - 2024-08-14

    ### Added
    - Add compatibility with jrl-cmakemodules workspace stack-of-tasks/eigenpy#485
    - Remove support of Python 3.7 stack-of-tasks/eigenpy#490

    ### Fixed
    - Remove CMake CMP0167 warnings stack-of-tasks/eigenpy#487
    - Fix compilation error on armhf stack-of-tasks/eigenpy#488
nim65s added a commit to nim65s/robotpkg that referenced this pull request Oct 11, 2024
    ## [3.10.0] - 2024-09-26

    ### Added

    - `GenericMapPythonVisitor`/`StdMapPythonVisitor` can now take an extra visitor argument in the `expose()` method, similar to `StdVectorPythonVisitor`

    ### Changed

    - Move `GenericMapPythonVisitor` to its own header `eigenpy/map.hpp`
    - Rename `overload_base_get_item_for_std_map` to `overload_base_get_item_for_map`, move out of `eigenpy::details` namespace
    - Move `EmptyPythonVisitor` to new header `eigenpy/utils/empty-visitor.hpp`

    ## [3.9.1] - 2024-09-19

    ### Added

    - Add test returning reference of std::pair stack-of-tasks/eigenpy#503
    - Add more general visitor `GenericMapPythonVisitor` for map types test `boost::unordered_map<std::string, int>` stack-of-tasks/eigenpy#504
    - Support for non-[default-contructible](https://en.cppreference.com/w/cpp/named_req/DefaultConstructible) types in map types stack-of-tasks/eigenpy#504
    - Add type_info helpers stack-of-tasks/eigenpy#502
    - Add NumPy 2 support stack-of-tasks/eigenpy#496

    ### Changed

    - Move `StdMapPythonVisitor` out of `eigenpy::python` namespace, which was a mistake stack-of-tasks/eigenpy#504

    ## [3.9.0] - 2024-08-31

    ### Changed
    - The `exposeStdVectorEigenSpecificType()` template function now takes the vector allocator as a template parameter stack-of-tasks/eigenpy#500

    ### Added
    - Add bp::dist to std::map converter stack-of-tasks/eigenpy#499

    ## [3.8.2] - 2024-08-26

    ### Fixed
    - Fix function signature on Windows stack-of-tasks/eigenpy#494

    ## [3.8.1] - 2024-08-25

    ### Fixed
    - Fix compatibility issue with NumPy 2.x on Windows stack-of-tasks/eigenpy#492

    ## [3.8.0] - 2024-08-14

    ### Added
    - Add compatibility with jrl-cmakemodules workspace stack-of-tasks/eigenpy#485
    - Remove support of Python 3.7 stack-of-tasks/eigenpy#490

    ### Fixed
    - Remove CMake CMP0167 warnings stack-of-tasks/eigenpy#487
    - Fix compilation error on armhf stack-of-tasks/eigenpy#488
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants