search

robotology / osqp-eigen

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

Restore compatibility with osqp version 0.6.0 #32

Closed GiulioRomualdi closed 5 years ago

GiulioRomualdi commented 5 years ago

This PR restore the compatibility with osqp version 0.6.0. When this PR is merged, it will be not possible to use osqp 0.5.0 along with osqp-eigen. Indeed https://github.com/oxfordcontrol/osqp/commit/78d11351555d6d4b0445a8d65d1066bb47df3b00 changes osqp_setup() interface. This problem should be taken into account since a custom version of osqp may be used by internal projects (e.g. https://github.com/robotology-dependencies/osqp/tree/fix-lf-problems)

See #31

traversaro commented 5 years ago

Some small suggestions (not directly related to this PR, to be honest):

osqp-eigen version osqp version
0.3 Compatible with osqp <= 0.5
0.4 Compatible with osqp >= 0.6
GiulioRomualdi commented 5 years ago

cc @traversaro

Some small suggestions (not directly related to this PR, to be honest):

  • Could you tag and release the last commit before the PR? It will be the last one compatible with osqp 0.5, and it contains a single commit, but quite important w.r.t. to relese 0.3.0 .
  • I would increase the CMake version, either in this PR or in a later commit.
  • Do you think it could make sense to mantain a table in the docs to document the relative compatibility between osqp and osqp-eigen version? Something like:

osqp-eigen version osqp version 0.3 Compatible with osqp <= 0.5 0.4 Compatible with osqp >= 0.6

Actually 0.3 is the current version of master while 0.4 is the version in devel. Probably before merging the PR I should merge devel into master By the way, as far as I understood is not possible to check the installed version of osqp from CMake

traversaro commented 5 years ago

By the way, as far as I understood is not possible to check the installed version of osqp from CMake

Upstream issue on this: https://github.com/oxfordcontrol/osqp/issues/195 .

Actually 0.3 is the current version of master while 0.4 is the version in devel. Probably before merging the PR I should merge devel into master

Mhh, good to know. independentily on what 0.3 or 0.4 are compatible with, such as table would help. Anyhow, considering that devel branch was never released, you can also merge the 0.6-compatibility in master, and then forward merge to devel.

GiulioRomualdi commented 5 years ago

Mhh, good to know. independentily on what 0.3 or 0.4 are compatible with, such as table would help. Anyhow, considering that devel branch was never released, you can also merge the 0.6-compatibility in master, and then forward merge to devel.

I don't know if this is a good idea. Right now the devel branch ensures back compatibility with master. In details, devel deprecate some functions defined in master.

On the other hand, when this PR will be merged the back-compatibility with a previous version of osqp will be broken. And I'm afraid that it will create problems -- see https://github.com/robotology-dependencies/osqp/tree/fix-lf-problems.

So to overcome possible issues, I would suggest to:

  1. Merge devel into master
  2. Merge this PR in devel (And probably change the version number from 0.4 to 0.4.1)
  3. Because of oxfordcontrol/osqp#195, it is not possible to check the version of osqp directly from CMake. Thus I will update the README.md describing the current status of osqp-eigen
  4. As soon as https://github.com/robotology/robotology-superbuild/issues/242 will be fixed, devel will be merged into master

What do you think @traversaro @S-Dafarra?

traversaro commented 5 years ago

As soon as robotology/robotology-superbuild#242 will be fixed, devel will be merged into master

We can fix immediately the version of osqp-eigen in the superbuild, so that you are free to work as you prefer, and we update the osqp and osqp-eigen version when dust has setted .

traversaro commented 5 years ago

Merge this PR in devel (And probably change the version number from 0.4 to 0.4.1)

To be honest, I think that if the required version of osqp changes, it is better to at least bump the minor version.

GiulioRomualdi commented 5 years ago

I'm updating the version from 0.4.1 to 0.5.0, at the same time I would like also to change the compatibility from AnyNewerVersion to SameMinorVersion

 include(InstallBasicPackageFiles)
 install_basic_package_files(${PROJECT_NAME}
   VERSION ${${PROJECT_NAME}_VERSION}
-  COMPATIBILITY AnyNewerVersion
+  COMPATIBILITY SameMinorVersion
   VARS_PREFIX ${PROJECT_NAME}
   NO_CHECK_REQUIRED_COMPONENTS_MACRO
   CONFIG_TEMPLATE ${CMAKE_SOURCE_DIR}/cmake/OsqpEigenConfig.cmake.in)

Unfortunately, this option is available only from cmake v3.11 and Ubuntu 18.04 supports at most cmake v3.10. @traversaro, do you know if there is a workaround that can be used?

traversaro commented 5 years ago

@traversaro, do you know if there is a workaround that can be used?

You can just vendor WriteBasicConfigVersionFile and its included files BasicConfigVersion-***.cmake.in in your project, it should work fine (I guess).

traversaro commented 5 years ago

You can just vendor WriteBasicConfigVersionFile and its included files BasicConfigVersion-***.cmake.in in your project, it should work fine (I guess).

Note that you will need to copy just the BasicConfigVersion-***.cmake.in that you use, and you need to modify the ${CMAKE_ROOT} in https://github.com/Kitware/CMake/blob/master/Modules/WriteBasicConfigVersionFile.cmake#L36 to ${CMAKE_CURRENT_LIST_DIR}.

GiulioRomualdi commented 5 years ago

@traversaro

You can just vendor WriteBasicConfigVersionFile and its included files BasicConfigVersion-***.cmake.in in your project, it should work fine (I guess).

Note that you will need to copy just the BasicConfigVersion-***.cmake.in that you use, and you need to modify the ${CMAKE_ROOT} in https://github.com/Kitware/CMake/blob/master/Modules/WriteBasicConfigVersionFile.cmake#L36 to ${CMAKE_CURRENT_LIST_DIR}.

Done in ac2a551

GiulioRomualdi commented 5 years ago

This PR is currently blocked by https://github.com/robotology/robotology-superbuild/pull/260