KumarRobotics / kr_mav_control

Code for quadrotor control
BSD 3-Clause "New" or "Revised" License
106 stars 42 forks source link

Modernize Cmake #124

Closed versatran01 closed 4 years ago

versatran01 commented 4 years ago

bump CMake version to at least 3.10 and use target-based CMake functions. pretty please? :crystal_ball:

kartikmohta commented 4 years ago

Ubuntu 16.04 has cmake 3.5.1, so that should be the maximum version that is used for now.

You mean using target-based cmake functions instead of things like set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -std=c++11 -Wall")? I would prefer something which just sets it at one place instead of per target for these kind of global flags.

tdinesh commented 4 years ago

We still use 14.04 on snapdragon platforms

jpaulos commented 4 years ago

What's driving the snapdragon Ubuntu version? I know 14.04 was LTS, but it's 5 years ended a year ago.

kartikmohta commented 4 years ago

What's driving the snapdragon Ubuntu version?

I suppose it's stuck at an old version because Qualcomm never bothered updating it and their drivers/software depend on 14.04.

versatran01 commented 4 years ago

You can always install latest cmake, takes two lines.

kartikmohta commented 4 years ago

You can always install latest cmake, takes two lines.

Hope you don't mean compiling cmake from scratch.

Also, cmake's add_compile_options/target_compile_options is supported in cmake v2.8.12 which is what 14.04 has, so don't really need to up the min version.

versatran01 commented 4 years ago
wget https://github.com/Kitware/CMake/releases/download/v3.17.0/cmake-3.17.0-Linux-x86_64.sh
chomd +x cmake-3.17.0-Linux-x86_64.sh
sudo ./cmake-3.17.0-Linux-x86_64.sh --prefix=/usr/local --skip-license

3 lines.

kartikmohta commented 4 years ago

How is just extracting files into /usr/local a good way to install anything? Also I don't see any ARM release.

Anyway, if you just want to replace set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -std=c++11 -Wall") with add_compile_options(-std=c++11 -Wall) then I'm ok with that. We can raise the min version to 2.8.12 for every package.

versatran01 commented 4 years ago

that's just an example, you could always put it wherever you want.

tdinesh commented 4 years ago

@versatran01 taking this person very seriously?

"there is something about writing find modules that makes my brain release endorphins. "

tdinesh commented 4 years ago
wget https://github.com/Kitware/CMake/releases/download/v3.17.0/cmake-3.17.0-Linux-x86_64.sh
chomd +x cmake-3.17.0-Linux-x86_64.sh
sudo ./cmake-3.17.0-Linux-x86_64.sh --prefix=/usr/local --skip-license

3 lines.

Multiply that doing it on 15 robots. Then spending 40-50 minutes per robot to compile all packages from scratch (of course you could cross-compile on your laptop, but then cross compile full ros indigo, maybe spend another day to do all of this in a docker, of-course this should have been done long time back, but here we are :) ..........................

versatran01 commented 4 years ago

@kartikmohta upgrading cmake has nothing to do with writing find modules, it actually reduces the .cmake files you need to write because they come with cmake now.

@tdinesh using docker is not a bad idea, maybe we should explore that. We already have a lab docker hub. But I feel you, this is why we can't have nice things.

kartikmohta commented 4 years ago

upgrading cmake has nothing to do with writing find modules, it actually reduces the .cmake files you need to write because they come with cmake now.

I never said anything like that :thinking:

I still didn't understand what real changes do you want in the CMakeLists.txt? "Modernize Cmake" is too broad a statement. Is there any big feature in cmake 3.10+ which would be useful?

versatran01 commented 4 years ago

https://cliutils.gitlab.io/modern-cmake/ The first few paragraphs explain why.

The other reason is since quadrotor_control is widely used across the lab, it becomes an example when people try to use cmake. I think it is better if quadrotor control follows the best practices of cmake, to set a good example.

kartikmohta commented 4 years ago

So the only big change that I see is the use target_compile_features instead of manually setting the CMAKE_CXX_FLAGS. But to add -Wall we still need to use add_compile_options/target_compile_options, right? Apart from that, there are things like adding PUBLIC/PRIVATE to target_link_libraries which would be good to do as well.

versatran01 commented 4 years ago

Yeah, https://github.com/lefticus/cpp_starter_project/blob/d7a2b6b891d75edc68c9d88dca7e24cd73e378b8/cmake/CompilerWarnings.cmake#L88

Yes, basically remove anything that is not a target_xxxx apart from the required catkin stuff. The exact cmake version is not a problem, but since snapdragon uses 14.04 I guess that's it.

kartikmohta commented 4 years ago

target_compile_options is available in cmake 2.8.12 so should be ok for 14.04. We can at least use that instead of target_compile_features for now.