CMake: Support building more than one precision #1

lukas-w · 2017-11-21T12:01:51Z

Adds support for enabling options ENABLE_FLOAT, ENABLE_DOUBLE (new and default), ENABLE_LONG_DOUBLE, ENABLE_QUAD_PRECISION at the same time, as discussed in https://github.com/ruslo/hunter/issues/1160.

Edit: Upstream PR at FFTW#117

ruslo

I've tried to build and install this code on my Linux machine and it's failed with some linker errors. On what platform you have tested it?

ruslo · 2017-11-21T13:02:33Z

CMakeLists.txt

-if (BUILD_SHARED_LIBS)
-  add_definitions (-DFFTW_DLL)
-endif ()
+  configure_file (cmake.config.h.in ${CMAKE_CURRENT_BINARY_DIR}/${fftw3_lib}/config.h @ONLY)


You've changed location of config.h file but I see no change of C++ code. Is it correct?

Yes, the target_include_directories in the line below accounts for this change, so that #include "config.h" still works.

ruslo · 2017-11-21T13:05:51Z

CMakeLists.txt

-  set (PREC_SUFFIX l)
-endif ()
+function (add_fftw_library PRECISION)
+  if (PRECISION MATCHES "SINGLE|LDOUBLE|QUAD")


It's better to use MATCHES "^(SINGLE|LDOUBLE|QUAD)$")

ruslo · 2017-11-21T13:07:52Z

CMakeLists.txt

+  if (PRECISION MATCHES "SINGLE|LDOUBLE|QUAD")
+    set (FFTW_${PRECISION} TRUE)
+    set (BENCHFFT_${PRECISION} TRUE)
+    if (PRECISION STREQUAL SINGLE)


You should update requirement to CMake 3.1+ or use string(COMPARE EQUAL ...):

https://cgold.readthedocs.io/en/latest/tutorials/control-structures.html#cmp0054

This problem looks like it will affect the if (PRECISION MATCHES "...") line too, so I guess it's better to update to 3.1.

ruslo · 2017-11-21T13:12:48Z

CMakeLists.txt

-  set (BENCHFFT_SINGLE TRUE)
-  set (PREC_SUFFIX f)
+if (BUILD_SHARED_LIBS)
+  add_definitions (-DFFTW_DLL)


GenerateExportHeader should be used instead:

https://cmake.org/cmake/help/latest/module/GenerateExportHeader.html

Example:

https://github.com/forexample/package-example/blob/a5ecaa520466ca286fb31e3fc0753901ae5ff0b4/Foo/CMakeLists.txt#L41-L46

https://github.com/forexample/package-example/blob/a5ecaa520466ca286fb31e3fc0753901ae5ff0b4/Foo/Source/foo/Bar.hpp#L6-L10

This is code I kept from the original CMakeLists.txt, so I'm hesitant to change it as it's not really related to the issue this PR is addressing. Or would you argue the change is necessary for clean use within Hunter?

As far as I understand this code will be used in installed headers too (can't check because I can't install). Meaning the burden of setting FFTW_DLL moved to user. GenerateExportHeader designed to resolve all problems.

Or would you argue the change is necessary for clean use within Hunter?

I'm pretty sure you we will hit this problem while testing.

I see, thanks for explaining. I tested this PR on Windows and didn't have any problems regarding export symbols, so I think it's fine this way. ffftw.h lines 78 to 87 handles this fine as far as I can see:

#if defined(FFTW_DLL) && (defined(_WIN32) || defined(__WIN32__)) /* annoying Windows syntax for shared-library declarations */ # if defined(COMPILING_FFTW) /* defined in api.h when compiling FFTW */ # define FFTW_EXTERN extern __declspec(dllexport) # else /* user is calling FFTW; import symbol */ # define FFTW_EXTERN extern __declspec(dllimport) # endif #else # define FFTW_EXTERN extern #endif

I can do some further testing. If it turns out not to be sufficient, I'll open a separate PR upstream.

Have you tried to install it and add it to user's project with find_package(fftw3 CONFIG REQUIRED)?

Yes, I successfully built and ran a project that depends on FFTW3 using Hunter on MSVC, so I did

hunter_add_package(fftw3) find_package(fftw3f REQUIRED)

I have the Hunter patches on my local machine and will submit them once this PR is settled.

FYI, I'm a developer on the LMMS project (https://github.com/LMMS/lmms). Adding FFTW3 (and some other packages) to Hunter is part of efforts to make LMMS compatible with MSVC. We plan on using Hunter to fetch dependencies on Windows.

Also, upstream still supports the autotools build system, so I don't think a GenerateExportHeader patch would be accepted because it could render the two systems incompatible.

hunter_add_package(fftw3)
find_package(fftw3f REQUIRED)

It should be with CONFIG:

hunter_add_package(fftw3) find_package(fftw3f CONFIG REQUIRED)

Note that my original question was about shared library (DLL on Windows). Hunter by default build static library, you have to add CMAKE_ARGS BUILD_SHARED_LIBS=ON:

https://github.com/ruslo/hunter/wiki/example.custom.config.id#custom-cmake-options

As a summary it's up to you. I'm just pretty sure this will lead to opaque linker error on user's side.

It should be with CONFIG:

Using CONFIG will break compatibility with non-Hunter builds.

Hunter by default build static library

Ah, I wasn't aware of that. I tried using BUILD_SHARED_LIBS=ON but didn't get any linking errors. Anyway, I'll test this on more environments in the context of LMMS and will submit another patch in case I run into problems.

Using CONFIG will break compatibility with non-Hunter builds

I think you're already using CONFIG implicilty.

ruslo · 2017-11-21T13:15:51Z

CMakeLists.txt

-  add_definitions (-DFFTW_DLL)
-endif ()
+  configure_file (cmake.config.h.in ${CMAKE_CURRENT_BINARY_DIR}/${fftw3_lib}/config.h @ONLY)
+  target_include_directories (${fftw3_lib} PRIVATE "${CMAKE_CURRENT_BINARY_DIR}/${fftw3_lib}")


Use $<BUILD_INTERFACE:...> here:

https://cmake.org/cmake/help/latest/manual/cmake-generator-expressions.7.html

This line is using PRIVATE, so $<BUILD_INTERFACE:...> won't make a difference here, as PRIVATE includes are only ever used during build, right?

ruslo · 2017-11-21T13:17:28Z

CMakeLists.txt

-set (subtargets ${fftw3_lib})
+  if (ENABLE_THREADS AND Threads_FOUND)
+    if (WITH_COMBINED_THREADS)
+      target_link_libraries (${fftw3_lib} ${CMAKE_THREAD_LIBS_INIT})


Threads::Threads available since CMake 3.1:

https://cmake.org/cmake/help/v3.1/module/FindThreads.html

Use it intead of CMAKE_THREAD_LIBS_INIT.

Same concerns as with GenerateExportHeader. Also, this will bump the minimum CMake version up to 3.1. I'm not sure if this is desired upstream. Ultimately, we'll want to be in sync with upstream so we can drop this fork, right? I can open a separate PR upstream though and see what they say. :)

Ultimately, we'll want to be in sync with upstream so we can drop this fork, right?

Also we want everything to work correctly. If it will not be accepted upstream, then just use fork.

Understood. I'll change it o use Threads::Threads as we'll update to CMake 3.1 anyway because of CMP0054.

ruslo · 2017-11-21T13:20:07Z

CMakeLists.txt

+
+
+  # pkgconfig file
+  set (prefix ${CMAKE_INSTALL_PREFIX})


I think this code, that doesn't depend on target, should be removed from function.

Hm… It doesn't depend on the target, that's right, but those are the input variables for the following configure_file(…) call, so I think we should still keep them immediately before that call for readability. No damage done except for a couple CPU cycles.

those are the input variables

There is also code below:

export (TARGETS ${fftw3_lib} NAMESPACE FFTW3:: FILE ${PROJECT_BINARY_DIR}/FFTW3LibraryDepends.cmake)

install(EXPORT FFTW3LibraryDepends NAMESPACE FFTW3:: DESTINATION ${CMAKE_INSTALL_LIBDIR}/cmake/fftw3${PREC_SUFFIX} COMPONENT Development)

Need to test if it will work correctly if called several times.

That code below you quoted differs on each call of the function though. ${fftw3_lib} evaluates to fftw3, fftw3f ,fftw3l or fftw3q depending on the precision selected.

lukas-w · 2017-11-21T13:34:51Z

@ruslo Thanks for reviewing. I'll address your points one by one.

The linker errors you get likely originate from missing code. The project has some generated C code that's not included in git, but is part of the official source tarball releases. Their CMake script finds these files using GLOB, so you'll get no error when they're missing. Instructions to build from the git repository are found in README.

The code they use for generating it is not portable and needs a Cygwin installation on Windows. So we'll likely have to create a release on GitHub and upload a source tarball that includes the generated files.

ruslo · 2017-11-21T13:49:01Z

Instructions to build from the git repository are found in README

Not sure I understand what step I missed. Also I don't see CMake instructions.

lukas-w · 2017-11-21T13:56:24Z

@ruslo The project's CMake script is fairly new and doesn't cover the whole building process. If you're not using the official source tarballs from their website (which includes the generated source files), you'll have to use the autoconf system to generate the source files first. The readme I linked contains instructions for this.

I guess the quickest way to test this PR is to download the offical source archive from http://fftw.org/fftw-3.3.7.tar.gz and just use that but override the CMakeFiles.txt by this PR's version first.

ruslo · 2017-11-21T14:05:11Z

doesn't cover the whole building process

How much effor it will take to cover this? Such peculiarity will affect maintaining this fork.

lukas-w · 2017-11-21T14:47:20Z

How much effor it will take to cover this?

A lot I guess. They use OCaml and ocamlbuild, which are rather large dependencies and don't have good support for Windows (as I said, only runs under Cygwin). Code generation takes a long time ~~and I haven't succeeded yet in running it myself~~. I really think we'd be better off just using the generated source code. It's the least painful and most portable way for us.

lukas-w · 2017-11-21T14:59:30Z

Here's an archive that includes generated source files and this PR's changes: fftw-3.3.7.tar.gz

ruslo · 2017-11-21T15:01:42Z

So can we add the generated code to repository so CMake build can be tested?

lukas-w · 2017-11-21T15:09:33Z

So can we add the generated code to repository so CMake build can be tested?

In theory, yes, but I'd suggest doing so only using GitHub's Release feature. Checking generated code into git will make syncing changes back upstream harder. Can we live with testing using the method I suggested for now? (Using the official tarball with modified CMakeLists.txt) Alternatively, I can gladly continue uploading new tarballs with more changes coming along.

In case you want to try the generation yourself: You'll need ocaml, ocamlbuild, fig2dev, autotools, indent and libtool. Then run sh mkdist.h.

ruslo · 2017-11-21T15:08:35Z

CMakeLists.txt

-  set (BENCHFFT_SINGLE TRUE)
-  set (PREC_SUFFIX f)
+if (BUILD_SHARED_LIBS)
+  add_definitions (-DFFTW_DLL)


I tested this PR on Windows and didn't have any problems regarding export symbols, so I think it's fine this way

You tested what exactly? You just run build of the project? Have you tried to install it and add it to user's project with find_package(fftw3 CONFIG REQUIRED)?

ruslo · 2017-11-21T15:12:55Z

CMakeLists.txt

+
+
+  # pkgconfig file
+  set (prefix ${CMAKE_INSTALL_PREFIX})


those are the input variables

There is also code below:

export (TARGETS ${fftw3_lib} NAMESPACE FFTW3:: FILE ${PROJECT_BINARY_DIR}/FFTW3LibraryDepends.cmake)

install(EXPORT FFTW3LibraryDepends NAMESPACE FFTW3:: DESTINATION ${CMAKE_INSTALL_LIBDIR}/cmake/fftw3${PREC_SUFFIX} COMPONENT Development)

Need to test if it will work correctly if called several times.

ruslo · 2017-11-21T15:16:59Z

Checking generated code into git will make syncing changes back upstream harder

No need to merge it to master, we can use separate branch for it. I just want to have simple way to test CMake build. I'm pretty sure the problems I've mentioned will break CI testing. Easy fetcheable code will save a lot of iterations for us.

* Update minimum version to 3.1 for CMP0054 new behaviour * Use stricter PRECISION regex matching * Use imported target Threads::Threads instead of CMAKE_THREAD_LIBS_INIT

lukas-w · 2017-11-21T15:34:56Z

@ruslo I addressed your points about CMP0054, Threads::Threads and MATCHES via ac2ad83. Pushed generated sources to lukas-w/[email protected]

CMake: Support building more than one precision

6b423d9

lukas-w mentioned this pull request Nov 21, 2017

CMake: Support building more than one precision FFTW/fftw3#117

Open

ruslo requested changes Nov 21, 2017

View reviewed changes

Minor CMake improvements

ac2ad83

* Update minimum version to 3.1 for CMP0054 new behaviour * Use stricter PRECISION regex matching * Use imported target Threads::Threads instead of CMAKE_THREAD_LIBS_INIT

ruslo changed the base branch from master to hunter-v3.3.7 November 21, 2017 16:22

ruslo merged commit bfe4993 into hunter-packages:hunter-v3.3.7 Nov 21, 2017

CMake: Support building more than one precision #1

CMake: Support building more than one precision #1

Conversation

lukas-w commented Nov 21, 2017 • edited Loading

ruslo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukas-w commented Nov 21, 2017

ruslo commented Nov 21, 2017

lukas-w commented Nov 21, 2017

ruslo commented Nov 21, 2017

lukas-w commented Nov 21, 2017 • edited Loading

lukas-w commented Nov 21, 2017

ruslo commented Nov 21, 2017

lukas-w commented Nov 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ruslo commented Nov 21, 2017

lukas-w commented Nov 21, 2017

lukas-w commented Nov 21, 2017 •

edited

Loading

lukas-w commented Nov 21, 2017 •

edited

Loading