Remove Catkin dependency while keeping CMake's find_package() feature #133
Conversation
| if(CATKIN_ENABLE_TESTING) | ||
| add_subdirectory(tests) | ||
| endif() | ||
| # FIXME |
There was a problem hiding this comment.
This PR can't be merged before this is fixed
| @@ -1,29 +1,11 @@ | |||
| cmake_minimum_required(VERSION 2.8.3) | |||
| cmake_minimum_required(VERSION 2.8.8) | |||
There was a problem hiding this comment.
This upgrade can be easily avoided in case that's wished: It's only required to generate the serialVersion.cmake file (using include(CMakePackageConfigHelpers) and write_basic_package_version_file()), but the contents of that file is trivial and could simply be perssited
There was a problem hiding this comment.
as usual it's too painful to upgrade Travis' environment - I'll get rid of this CMake version upgrade requirement
| - make install_deps | ||
| - source setup.bash | ||
| script: | ||
| - make && make test | ||
| - make |
There was a problem hiding this comment.
FIXME: For the time being tests are not built, so they cannot be run
|
@wjwwood I'm now quite happy with this PR - and the build is passing Let me know what you think about it - if you're willing to merge this then I'll invest the time to have CMake build the tests again |
| @@ -0,0 +1,21 @@ | |||
| set(PACKAGE_VERSION "1.2.1") | |||
There was a problem hiding this comment.
Currently with Catkin, any newer version of this package is considered compatible.
With this version file, the package is compatible only if:
- the requested version number is equal to the installed one
- or, the requested version number is inferior to the installed one AND the major version number matches (for "1.2.1", the major version number is "1")
So, if "0.9" is requested and "1.2.1" is installed => mismatch
If "1.1" is requested and "1.2.1" is installed => match
Relying on the major version number for compatibility seems to me like a good idea, and enables breaking changes to be introduced in order to keep this package alive [relates to #102 for instance].
|
Looking forward to the next move, I really think it deserve at least a branch without catkin as dependency since there are so many people need this feature, and many of the forks are just doing this particular thing, I really do not think it a good idea for this library to have so many variations just because this problem, it is like a Tower of Babel and finally will do no good to this library since many people, like me, feel not so good to use a forked branch for developments, and chances are that they will try to look for alternatives if they are showing up (but far, no suitable choices 😄 ). And please make it Anyway, changes will take time and will have a lot of backward compatibility works for you guys, before that I will just use this PR and waiting. :) Regards, --MiaoDX |
|
As I mentioned in the other two pull requests about this issue, my plan is to use CMake only (much as this pr does) going forward, but I want to leave the current version with catkin. I do not, however, want to just take this pr as-is, because I want to keep the package.xml and use exported targets. However, I will try to use the commits from this pr if I can to give you some attribution for working on this. |
From a Catkin project, it is straightforward to add a dependency on a "normal" CMake package. However, the opposite is not true.
This PR relates to #123 and #111 , but with the following 2 changes:
find_package()feature is preservedHere is how a project can depend on this library: https://github.com/dbolkensteyn/serial-lib-consumer
I've tested that one both on Linux and Windows - but I do not have an Mac OS X machine available at the moment.
I wanted to keep this PR small, but IMO the following should be done afterwards:
Makefile,visual_studioas well asserial.sublime-project: Those files should not be persisted in the SCM, as they are supposed to be generated by CMakeREADME.mdinstructions - relates to Update build and install instructions #114CHANGELOG.rstwithchanges.txt?Also, this PR isn't ready to be merged just yet: For instance, I've still have to re-enable the support for testing.