Unbreak export for CMake 3.16 and Python-less consumers - Githubissues

rdiankov / openrave

Open Robotics Automation Virtual Environment: An environment for testing, developing, and deploying robotics motion planning algorithms.

http://www.openrave.org

Other

693 stars 343 forks source link

Unbreak export for CMake 3.16 and Python-less consumers #1298

Closed PeterBowman closed 1 year ago

PeterBowman commented 1 year ago

PR https://github.com/rdiankov/openrave/pull/1224 introduced a few regressions in the CMake config file for exports:

A set_and_check(OpenRAVE_PYTHON_DIR) macro call that breaks for configurations that have Python bindings disabled. I suggest to remove this check since the exported variable is set within the preceding if clauses anyway.
A pair of add_library(libopenrave ALIAS ...) instructions that issue an "add_library cannot create ALIAS target "libopenrave" because target "OpenRAVE::libopenrave" is imported but not globally visible." fatal error on CMake 3.16 (i.e. Ubuntu 20.04; it might not break at later releases, see this thread). It is customary to prepend the namespace prefix to the target name in downstream projects, hence the alias would render redundant. If the intention was to allow parts of OpenRAVE itself to conditionally consume local and imported targets, then the alias should be created within OpenRAVE; for instance in src/libopenrave/:
```
add_library(libopenrave SHARED ${openrave_lib_SOURCES})
add_library(OpenRAVE::libopenrave ALIAS libopenrave) # add this if needed
```
The set( OpenRAVE_LIBRARIES libopenrave ) instruction adds a faulty link target without the library suffix. I suggest to move this to the "Backward compatibility only" section next to OpenRAVE_CORE_LIBRARIES, and query the proper binary location from the modern target in a similar fashion.

I would also suggest (not in the scope of this PR) using find_dependency instead of find_package.

cc @undisputed-seraphim

rdiankov commented 1 year ago

@cielavenir can you check this out? thanks

cielavenir commented 1 year ago

-----BEGIN PGP MESSAGE-----

hQIMAwAinXiLUbjzAQ/+M8miuY6q7zsLVM4amUFjLmJ+P+wD5pExC4zkG28Ssyq+
AlPS2x/yehX64VQS15Z4Y6Ip3OLZtOUQ8oxB7qbiSDVWGe1NAPKSBvtjDjzMGzTo
iRcOsxH7/4tA9DUqOECUIbQnyVKi59NZ5XgwpAgu3EWrCm2CLPapOBgRMx1TgD2Y
EJgLReyYptEtLaPtoTQmrgsRB5FdBvcelUxbq3CCiHV7lWxK1hHT/pWJx+IfzIsS
D8PtNq6HlHp7wNo1rmQybc7+7wdmjV5BbB3AXx8uMBdemtprS8xqGNtV9qcpuGvZ
4qHrbROTK3boRWFgVkBvzte5LSHqu8q7fT6t8hLLVUniUQvq9D3ShiSLlX43t3jJ
NJ2zJgeZuVSOxgZrZRKPw5MvpGe/rRWGRBMyYqKeURgKUrg9/VvHmzEy6UfsywhI
Hm81exDKFD+Q6m/I55WcRoWg/8ZcdQO4amWtfV2iQhLRMQzXAG+Qi6nzlhZbd7hr
tn1vWlXLyd6CFdDH7kCWp3Evj6GhnZU8R4K6TAiUKY7RllZWiPT/OMNQDeWHiOmk
EgKcb/ENGaUMBviqNDbZ+xXj3nQUjNHRneyhqbbJ3NPN/RD04+c7jCQ+nr5mFo15
ueGqOthsIkVqQf73cXsMBeglZJ8Pba2AgXyG0tsecYPVr+PfLceH3msrpy7+XOHS
ggEEgIwbkAksbNODwVRoJVfUg0ABkbhEMKOODt0EIjrwQuDGv09oFfx1sSMLzybH
65Jord32VDBtIjBc5AV4j6/+DDy94kLiixqaxvsT7vC75wMv9CnAjTz/L3WIpsZA
9AZIG4hur1we7N+kdD9QiYSo5ylsdG5DPvDUjNU6E5dXFc8=
=Bumb
-----END PGP MESSAGE-----

cielavenir commented 1 year ago

1: It is correct that there is no need to set_and_check(OpenRAVE_PYTHON_DIR)

2: By using separate ubuntu_focal environment and checking some versions from https://apt.kitware.com/, I found that cmake 3.18.0 is required for add_library(libopenrave ALIAS ...) Also, I grepped mujin CMakeLists and nothing used "libopenrave" literal other than openrave itself's CMakeLists. Maybe ALIAS can be removed.

3: I think set( OpenRAVE_LIBRARIES libopenrave ) works as long as add_library(libopenrave ALIAS ...) is used.

Actually, for that part, I prefer set( OpenRAVE_LIBRARIES OpenRAVE::libopenrave )

cielavenir commented 1 year ago

set( OpenRAVE_LIBRARIES OpenRAVE::libopenrave )

@PeterBowman if that's ok for you, we can merge your changeset.

cielavenir commented 1 year ago

-----BEGIN PGP MESSAGE-----

hQIMAwAinXiLUbjzARAAkokN8oz6YQT0kJSSPKB0FyuHwnOvIr4OPodTrlR0DaMz
Yv+jyFNSE7OLFdODe0ARGLwYFcUK6AQLOa9jMlGfes0hIwqN9MqjUjU3lybtKMAs
S0DTLCurP+FqtzbhWcC0IzL5bA/tL8Vy76dG4SbH3Spk4fb26PGWJvGRsDp/o+9Z
h7kIBiPdJXt0ACaubGrIdDtMJpsss8JgMJdt519wnbzOcNqBoYuW/K2Btm787VRo
2HDJVmW+o1sPIv+s5IHLMGlXQB9P7YKqlQjdLINu9S846KiUnR+XEVa6pavMW7fJ
iqZme7LT2SbOBRa7sJVQZKTlSR1ZMuRu73G2jeeyfJobKzIyeSx8gcbyFj/Hs4jZ
HAIGzGxpNiyozDCR39DFrA/mZbnu0XQ2DiqU+lMQAe9rB/JTwLXokrvG7rgrOq97
2udUsU5IkASbKycg0jqGOlqAQgt8UtceKrjrkOPLhcnRcJ18ORq8xNqElaQbAyea
hQoAxuLKsXvOMDENRFiPeh5AlRaGaQs8esjpQd9MCrZ5TA0DeZBHwUlC4qfgXv5F
P2kJXzKBue0+8x9yVJWpAB28zZer43n0J9YvCU/bTFWAgSmZDaY7K+WRIdMDLhVS
UMaJWgbmeYk1RAz80yKvzXQ4VolIMG9gvXuFQWphFSLcyIXnatkXdnX4qoFdZT3S
ggG5Ee+A5ESGmCIIJ4qs6HBd4Cg2AewZeYuiGq1d+W6/h2kS+2E0rGSvRDQvM65d
cIaBJHrLxQhdclqIFi1m0mKANKVYa8W6yKBm7JdTd2MDs4lyIFcam8TlxR4+K8yh
thLh77MAkl+68aw3MGf1VJ+XNC7jyu8fjL6bjO5Rcz32aNU=
=zEgy
-----END PGP MESSAGE-----

rdiankov commented 1 year ago

Thanks!

cielavenir commented 1 year ago

those two pipelines are passing (paste the message to mugpg -d to get the plain URL)