cmake: Update to Modern CMake usage #2230

noc0lour · 2018-12-15T22:29:52Z

This is still very much a work in progress. But the main parts are working (building libs & python bindings).

During the update some wrong includes (brackets instead of quotes) were found.
Static libs & swig building still needs some love.

convert add_definitions
split out target_sources()

noc0lour · 2018-12-15T22:35:04Z

cmake/Modules/GnuradioConfig.cmake.in

-# if GR_REQUIRED_COMPONENTS is not defined, it will be set to the following list (all of them)
-if(NOT GR_REQUIRED_COMPONENTS)
-  set(GR_REQUIRED_COMPONENTS RUNTIME ANALOG BLOCKS DIGITAL FFT FILTER PMT)
-endif()


Removing all of this was "just" a test, we shouldn't actually remove all of it for compatibility for at least 3.8.

So will you be adding this back in for the actual PR once it's done?

We need to discuss/think about how to handle "old" and "modern" CMake. Or if we just require for 3.8 that downstream libraries/packages have to use modern CMake.

From what I've learned/read it's near impossible to support the old variable exporting approach with targets. Basically one would have to render all include paths embedded in exported targets during generation time of the GnuradioConfig.cmake. I can't seem to find a way of doing that.

I would like to stick with the 3.5.1 CMake version we currently have on master.

...unless this totally screws up this PR.

noc0lour · 2018-12-15T22:39:56Z

Well, looks like I need to point my things to the right volk commit (which is not yet in master)'

noc0lour · 2018-12-16T00:18:19Z

OK, now builds and runs all tests. Due to rebasing the volk submodule on master with a hack commit on top it nows fails the off-by-one volk tests.

gr-analog/lib/CMakeLists.txt

gr-channels/lib/CMakeLists.txt

gr-digital/lib/CMakeLists.txt

gr-filter/lib/CMakeLists.txt

gr-trellis/lib/CMakeLists.txt

gr-wavelet/lib/CMakeLists.txt

noc0lour · 2019-01-03T22:21:01Z

This is definitely very much work-in-progress, so comments on everything / pointing out bugs and things to do is appreciated. It's basially compiling if the CMake version is new enough.

michaelld

I'll take changes I see even before testing. Mostly whitespace for now.

noc0lour · 2019-01-03T22:35:27Z

Oh, sure, if you can pass me the patch point out commits I can include/factor in, that would be great. The cmake I wrote is not very clean, whitespace-wise.

michaelld

Lots of whitespace changes & guessing some I didn't catch; some changes I think should be in a different PR; nothing critical. All reads nicely & I love the code simplification & hierarchal nature of "modern CMake" and how you're using it here.

.gitmodules

michaelld · 2019-01-03T22:27:45Z

cmake/Modules/FindMPLIB.cmake

+    INTERFACE_INCLUDE_DIRECTORIES "${MPLIB_INCLUDE_DIRS}"
+    INTERFACE_LINK_LIBRARIES "${MPLIB_LIBRARIES}"
+  )
+endif()


Do we have a style GREP for CMake? Some "endif()" statements are empty, while others replicate the "if()" section. I'd rather see consistency in commits, if we require it. If not then I prefer empty such as this one.

I think we don't have a CMake style GREP or some other guide. Would be nice to nail it down to one definitve right solution.

michaelld · 2019-01-03T22:29:01Z

cmake/Modules/FindThrift.cmake

+  set_target_properties(Thrift::thrift PROPERTIES
+    INTERFACE_INCLUDE_DIRECTORIES "${THRIFT_INCLUDE_DIRS}"
+    INTERFACE_LINK_LIBRARIES "${THRIFT_LIBRARIES}"
+    )


Do we have a style GREP for CMake? The trailing ")" here I think should be 2 spaces back to match the start character of "set_target_properties". Sometimes we do this and sometimes we don't, even in the code immediately preceding this addition.

This has been bugging me as well, my editor is trying to align everything with two spaces.

michaelld · 2019-01-03T22:29:37Z

cmake/Modules/GnuradioConfig.cmake.in

-# if GR_REQUIRED_COMPONENTS is not defined, it will be set to the following list (all of them)
-if(NOT GR_REQUIRED_COMPONENTS)
-  set(GR_REQUIRED_COMPONENTS RUNTIME ANALOG BLOCKS DIGITAL FFT FILTER PMT)
-endif()


So will you be adding this back in for the actual PR once it's done?

michaelld · 2019-01-03T22:31:41Z

cmake/Modules/GnuradioConfig.cmake.in

-if(NOT GR_REQUIRED_COMPONENTS)
-  set(GR_REQUIRED_COMPONENTS RUNTIME ANALOG BLOCKS DIGITAL FFT FILTER PMT)
-endif()
+include(CMakeFindDependencyMacro)


I love how concise this file will be! Do we really need this "include" to get the new code working? All of the CMake commands like stock / normal, like they don't need any externally included special sauce. I won't know until I can test this whether it's actually required, but maybe you know?

find_dependency apparently needs to include to work. For package config cmake files it's advised to use find_dependency instead of find_package since it's returns on failure and then stops processing.

gr-fec/lib/ldpc_bit_flip_decoder_impl.cc

gr-fec/lib/ldpc_gen_mtrx_encoder_impl.cc

gr-fec/lib/polar_decoder_sc_list.cc

gr-fec/lib/scl_list.cc

marcusmueller · 2019-01-04T18:36:20Z

Regarding the VOLK dependency: obviously, we'll have to update VOLK for 3.8, so, can you do a similar PR for your VOLK changes against gnuradio/volk?

noc0lour · 2019-01-05T16:00:01Z

@marcusmueller yes, for volk Maitland already has send me & @n-west a path. I'll post it in a PR.

noc0lour · 2019-01-12T17:26:42Z

The exported targets need to be created per component, with a generated whitelist during configuration.
https://cmake.org/cmake/help/v3.5/manual/cmake-packages.7.html#creating-a-package-configuration-file
The first config.cmake is just a rough idea how it would work for super simple packages.

noc0lour · 2019-02-03T20:39:04Z

Two key questions that came to my mind:

What's the use case for building a shared & static library in the same build process ( in fact it's compiling all source files twice anyway). If that's not a real use case (but leftovers from autotools migration) we can significatly simplify the cmake syntax by not specifying the library type at the add_library() call, but with a default cmake switch BUILD_SHARED_LIBS. Some options still have to be special cased for static/shared, but it would remove a lot of uneeded cruft from cmake files as well.
Is there a usecase for using libtool versioning, and when did someone use it the last time to see if it works (whatever that means) ?

noc0lour · 2019-02-07T17:19:10Z

@skoslowski Now all component libraries (if I didn't forget any) are created with their "base" sources and additional sources are added with target_sources(). Also defines are now added with target_compile_definitions in component CMakeLists. In the toplevel CMakeLists there is still a whole lot of global defines I don't know what to do with. Same for ConfigChecks.cmake
But should be buildable & installable in the current state.

skoslowski · 2019-02-07T20:55:56Z

Awesome, great work

jdemel · 2019-02-17T13:09:27Z

@noc0lour in this mail you stated that modern cmake is not really a 3.8 feature because it did not get included as part of the feature freeze. Though, #2075 does essentially depend on your work here. As far as I can tell, updates to gr-modtool and by extension to fix #2075, depend on your work in order to make the OOT 3.8 transition as smooth as possible.
This is just my opinion/observation of the topic. Nevertheless, I see a need to clarify on that.

noc0lour · 2019-02-17T13:55:09Z

@jdemel Ultimately it's not my call to make, also I couldn't promise it will be ready "on time". But after confering with @marcusmueller I'll add it to the 3.8 project for inclusion.

mbr0wn · 2019-02-23T00:48:22Z

Do we need CMake 3.8? @marcusmueller and I picked 3.5.1 because it's available in 16.04, which is still relevant.

mbr0wn

Let's clean up history and merge. We probably have details to figure out which will become obvious after the merge, but that's OK.

mbr0wn · 2019-02-23T19:18:52Z

Ignore my comment. I mixed up GR and CMake versions.

noc0lour · 2019-02-24T00:41:04Z

@mbr0wn cmake 3.8 is still needed nonetheless. I had a offline chat with Marcus about that and he was OK with that iirc.

drmpeg · 2019-02-24T08:26:49Z

Since Cmake 3.8 is required, maybe it should be set in the top level CMakeLists.txt? Otherwise, on Ubuntu 16.04 you get many "list does not recognize sub-command FILTER" errors that isn't as useful as just:

CMake Error at CMakeLists.txt:30 (cmake_minimum_required):
  CMake 3.8 or higher is required.  You are running version 3.5.1

noc0lour · 2019-02-24T17:06:13Z

@mbr0wn there is not a lot to squash here, I think it would be best to use a explanatory merge commit.to have a single commit in the master branch but to preserve the logical structure of the changes.

Previously lib/ was in the include path and thus the compiler was able to find `dvb/dvb_defines.h` in the path. Now it needs a relative path to be found as local include.

This updates the volk submodule to the point where volk switched to modern CMake with exported compile targets.

This includes using target based setting of includes and link libraries. This will transitively add the includes and linking flags to dependent targets. This is still a work in progress since only the dynamic libraries have been touched and not all of include_directories directives are gone yet. cmake: remove GR_INCLUDE_SUBDIRECTORY macro Previously this macro was used to inject subdirectories in the current CMake namespace. This is generally undesired and pollutes the current context. previously GNU Radio CMake had a non-default option ENABLE_STATIC_LIBS to build both, shared libraries and static libraries. This seems to be a construction taken over from autotools and serves no purpuose in CMake and complicates the library building. cmake: remove GR_LIBTOOL and la generation support This looks like it was primarily used to support projects using autotools, but comments state that the generated .la files aren't compatible with autotools anyway. cmake: Bump required CMake version to 3.8 UseSWIG cmake uses syntax which requires at least CMake 3.8 and is non-trivial to change

noc0lour added CMake Don't Merge labels Dec 15, 2018

noc0lour force-pushed the update_cmake branch from 240ab07 to 9e6e905 Compare December 15, 2018 22:31

noc0lour commented Dec 15, 2018

View reviewed changes