robotology / osqp-eigen

Simple Eigen-C++ wrapper for OSQP library
https://robotology.github.io/osqp-eigen/
BSD 3-Clause "New" or "Revised" License
385 stars 114 forks source link

Header files added to library #116

Closed mhubii closed 2 years ago

mhubii commented 2 years ago

Why are the header files added to the library target? https://github.com/robotology/osqp-eigen/blob/83812bd0a56bbb656cac7016b307845e4a0ed11e/CMakeLists.txt#L108

Should the location of these headers https://github.com/robotology/osqp-eigen/blob/83812bd0a56bbb656cac7016b307845e4a0ed11e/CMakeLists.txt#L96 not be prefixed with the current source dir, ie ${CMAKE_CURRENT_SOURCE_DIR}? Because otherwise the install might look for headers were there are no https://github.com/robotology/osqp-eigen/blob/83812bd0a56bbb656cac7016b307845e4a0ed11e/CMakeLists.txt#L116

traversaro commented 2 years ago

Why are the header files added to the library target?

Mainly for users of ÍDEs to have the headers file listed as part of the target. What is the problem of having the header files added in the target?

not be prefixed with the current source dir, ie ${CMAKE_CURRENT_SOURCE_DIR}?

In all the cases we ever had it was not necessary to prefix the files with ${CMAKE_CURRENT_SOURCE_DIR}, even for libraries that we use with FetchContent (for example https://github.com/robotology/idyntree/blob/59b2a6186c5011c1d9f35784fced816654602cb8/src/core/CMakeLists.txt). Can you explain in which context this is generating a problem and reporting the related error?

mhubii commented 2 years ago

Mainly for users of ÍDEs to have the headers file listed as part of the target. What is the problem of having the header files added in the target?

there is no problem associated with it, but don't understand the necessity for it. But no worries. Just curious.

In all the cases we ever had it was not necessary to prefix the files with ${CMAKE_CURRENT_SOURCE_DIR}, even for libraries that we use with FetchContent (for example https://github.com/robotology/idyntree/blob/59b2a6186c5011c1d9f35784fced816654602cb8/src/core/CMakeLists.txt). Can you explain in which context this is generating a problem and reporting the related error?

two wrong doesn't make one right. This error occurs when you fetch osqp-eigen as part of a colcon build in ROS2.

traversaro commented 2 years ago

two wrong doesn't make one right.

I just mentioned one case that was working fine with FetchContent, so I do not see anything "wrong" here. It is also great if you could keep a positive attitude while discussing here, thanks! :)

This error occurs when you fetch osqp-eigen as part of a colcon build in ROS2. @traversaro

Can you provide a way to reproduce for the error? If that is complex, can you provide at least the error that you are having? Thanks!

traversaro commented 2 years ago

If the issue is not related to the header files added to library, probably we can rename it?

mhubii commented 2 years ago

yes sorry, I am under a lot of pressure recently, and have to finish up some things. Thanks for providing this project in the first place! I'll try to provide you with a minimal example, but would have to dig deeper to understand what is happening in detail. One second

traversaro commented 2 years ago

yes sorry, I am under a lot of pressure recently, and have to finish up some things.

Sure, no problem.

I'll try to provide you with a minimal example, but would have to dig deeper to understand what is happening in detail. One second

Even without a minimal example, having the cmake/colcon output with the error is already quite useful.

mhubii commented 2 years ago

here are the steps to re-produce

mkdir -p tmp_ws/src && cd tmp_ws/src && \
git clone --branch header-fault git@github.com:mhubii/osqp_vendor.git && cd .. && \
colcon build

the error should be something like

--- stderr: osqp_eigen_vendor                              
CMake Error at cmake_install.cmake:99 (file):
  file INSTALL cannot find
  "/tmp/tmp_ws/src/osqp_vendor/osqp_eigen_vendor/include/OsqpEigen/OsqpEigen.h":
  No such file or directory.

note that #115 is already fixed in this osqp-eigen fork. The header-fault branch of the osqp-eigen fork https://github.com/mhubii/osqp-eigen/tree/header-fault will be cloned by CMake fetch

mhubii commented 2 years ago

again, I apologize for coming across rude, I am very thankful that you provide this project

traversaro commented 2 years ago

I suspect that the problem is in https://github.com/mhubii/osqp_vendor/blob/header-fault/osqp_eigen_vendor/CMakeLists.txt#L24 . You are installing again OsqpEigen target that is already installed in https://github.com/robotology/osqp-eigen/blob/255305d8c6fd999db66745cef4758d75cbfa61ba/CMakeLists.txt#L130 . A possible alternative strategy is to define your own target and just make sure that it links transitevely on OsqpEigen, for example with this patch:

diff --git a/osqp_eigen_vendor/CMakeLists.txt b/osqp_eigen_vendor/CMakeLists.txt
index 6dfee01..645ae4e 100644
--- a/osqp_eigen_vendor/CMakeLists.txt
+++ b/osqp_eigen_vendor/CMakeLists.txt
@@ -18,12 +18,15 @@ FetchContent_Declare(

 FetchContent_MakeAvailable(OsqpEigen)

-ament_export_targets(OsqpEigenTargets HAS_LIBRARY_TARGET)
+ament_export_targets(OsqpEigenVendorTargets HAS_LIBRARY_TARGET)
-ament_export_dependencies(osqp_vendor eigen3_cmake_module Eigen3)
+ament_export_dependencies(osqp_vendor eigen3_cmake_module Eigen3 OsqpEigen)

+add_library(osqp_eigen_vendor INTERFACE)
+target_link_libraries(osqp_eigen_vendor INTERFACE OsqpEigen)
+
 install(
-  TARGETS OsqpEigen
-  EXPORT OsqpEigenTargets
+  TARGETS osqp_eigen_vendor 
+  EXPORT OsqpEigenVendorTargets
 )

 if(BUILD_TESTING)
mhubii commented 2 years ago

okay will do, thanks

mhubii commented 2 years ago

actually this doesn't work. On compilation the following error is thrown

CMake Error at CMakeLists.txt:6 (find_package):
  Found package configuration file:

    /tmp/ws/install/osqp_eigen_vendor/share/osqp_eigen_vendor/cmake/osqp_eigen_vendorConfig.cmake

  but it set osqp_eigen_vendor_FOUND to FALSE so package "osqp_eigen_vendor"
  is considered to be NOT FOUND.  Reason given by package:

  The following imported targets are referenced, but are missing:
  OsqpEigen::OsqpEigen
traversaro commented 2 years ago

Strange, the /tmp/ws/install/osqp_eigen_vendor/share/osqp_eigen_vendor/cmake/osqp_eigen_vendorConfig.cmake file should contain the find_package(OsqpEigen) that should define that target.