Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CMake: Support building more than one precision #117

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

lukas-w
Copy link

@lukas-w lukas-w commented Nov 21, 2017

This patch adds support for enabling the CMake options ENABLE_FLOAT, ENABLE_DOUBLE (new and on by default, builds the standard double version of fftw), ENABLE_LONG_DOUBLE, ENABLE_QUAD_PRECISION at the same time.

With the old behaviour, you have to invoke CMake & make multiple times, once for each precision you want to build, even though this wasn't documented anywhere. If you'd still pass more than one of the options, one would override the other, while contradicting compile definitions (such as FFTW_SINGLE and FFTW_LDOUBLE) would be enabled at the same time.

With this patch, add_library will be called once for every precision option passed.

This change was necessary to integrate fftw with the C++ package manager Hunter, and merging it would remove the need for the Hunter project to maintain a fork, see https://github.com/ruslo/hunter/issues/1160 and hunter-packages#1.

Most distros (such as Ubuntu) provide all versions of fftw, so maybe this will make packaging easier for them too.

Edit: Added some minor enhancements via ac2ad83: Changed minimum CMake version to 3.1 to use the new if behaviour (see CMP0054 and https://cgold.readthedocs.io/en/latest/tutorials/control-structures.html#cmp0054).
Beginning with CMake 3.1, FindPackage(Threads) creates an imported target Threads::Threads, which is preferred over ${CMAKE_THREAD_LIBS_INIT}, so I changed that as well.

* 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
@hjmjohnson
Copy link

@lukas-w I would recommend killing this pull request and starting a new one. It appears that this pull request is partially merged already.

@lukas-w
Copy link
Author

lukas-w commented Jan 10, 2018

@hjmjohnson It says no conflicts for me:

image

What do you mean by "partially merged"? As far as I can see, there haven't been any changes to CMakeLists.txt on master since this PR was opened.

@hjmjohnson
Copy link

@lukas-w there are no merge conflicts, but in the stream above this message there is a purple "Merged" tag for items up to "485fbc0"

@lukas-w
Copy link
Author

lukas-w commented Jan 10, 2018

image

This one? That's just GitHub showing cross-references. The "merged" tag refers to a different Pull Request, hunter-packages#1.

@hjmjohnson
Copy link

OK. In an attempt to get this reviewed and potentially integrated, I was trying to make the suggestion that you make the review process easier.

There is nothing technically wrong with you merge request, it is just more difficult to evaluate due to the multiple dependencies and merges.

Feel free to ignore my suggestion that simplifying the pull request to a simple,single,non-merged , single topic patch will ease the review process and increase the chances of this being accepted quickly.

I hope these suggestions are approved. I'd like to see them incorporated.

@jakirkham
Copy link

Would you be able to look at this @xantares?

@xantares
Copy link
Contributor

xantares commented Mar 12, 2018

In all cases all object file must be regenerated as definitions changes.
In the case of debian if you look at their compile scripts they have to recompile anyways as they have custom flags for each flavour, which is not supported here.
https://packages.ubuntu.com/source/xenial/fftw3
Also I think it's better to mimic the autotools behavior.

This doesnt bring much to the table while making CMakeList even less readable.
AFAIK I'd rather keep it simple ie add a simple warning if contradicting options are passed, which is a concern made here.
-1

Could you describe why it was "necessary" in the first place ?

@dzenanz
Copy link

dzenanz commented Jun 12, 2019

Could you describe why it was "necessary" in the first place ?

The build process has to be run multiple times. It is better if running build "once" would be enough. "Once" meaning once per Debug/Release/RelWithDebInfo configuration.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants