mborgerding / kissfft

a Fast Fourier Transform (FFT) library that tries to Keep it Simple, Stupid
Other
1.46k stars 283 forks source link

build: Install headers #39

Closed jtojnar closed 3 years ago

basilgello commented 3 years ago

@mborgerding How about switching to CMake fully and dropping Makefile support? Of course I can do overhaul both build systems and test it on Windows, Linux and maybe FreeBSD, but decided to ask before doing the actual work.

mborgerding commented 3 years ago

@mborgerding How about switching to CMake fully and dropping Makefile support? Of course I can do overhaul both build systems and test it on Windows, Linux and maybe FreeBSD, but decided to ask before doing the actual work.

I don't want to drop Makefile support until cmake becomes more univerally available. e.g. I had a CentOS 8 development machine built up for 8 months before I found a need to install cmake. That is a full-featured modern linux distro. We have a lot of users on embedded systems with minimal toolchains.

I understand the advantages of consolidating configuration as much as the next guy. I love the DRY principle almost as much as the KISS principle, but no.

consistency

@jtojnar , would it make sense to have the make install headers and libs go in the same place as the debian install?

version numbers?

While we're on the subject, what about versioning the lib.so? Anybody know how to go about that?

basilgello commented 3 years ago

I don't want to drop Makefile support until cmake becomes more univerally available. e.g. I had a CentOS 8 development machine built up for 8 months before I found a need to install cmake. That is a full-featured modern linux distro. We have a lot of users on embedded systems with minimal toolchains.

I understand the advantages of consolidating configuration as much as the next guy. I love the DRY principle almost as much as the KISS principle, but no.

Got it! I can then fix Makefile separately in the same PR.

version numbers?

While we're on the subject, what about versioning the lib.so? Anybody know how to go about that?

I suggest keeping ABI version and soname consistent with Debian (libkissfft-float.so.131) but tag the 131.1 release that can be imported everywhere. My friend and colleague Mattia can add more on it.

jtojnar commented 3 years ago

@jtojnar , would it make sense to have the make install headers and libs go in the same place as the debian install?

What do you mean? $(PREFIX)/include is the standard location: https://www.gnu.org/prep/standards/html_node/Directory-Variables.html

mborgerding commented 3 years ago

@jtojnar , would it make sense to have the make install headers and libs go in the same place as the debian install?

What do you mean? $(PREFIX)/include is the standard location: https://www.gnu.org/prep/standards/html_node/Directory-Variables.html

By default, the cmake installer puts all the .h files in /usr/local/include , while the make install puts many of them under /usr/local/include/kissfft.

basilgello commented 3 years ago

By default, the cmake installer puts all the .h files in /usr/local/include , while the make install puts many of them under /usr/local/include/kissfft

This can be easily fixed since kissfft is,still in unstable now and only a couple of Kodi addons depend on it. I can fix it in 131.1 release if we come to concensus on this question. Mattia will join us tonight I hope.

mapreri commented 3 years ago

By default, the cmake installer puts all the .h files in /usr/local/include , while the make install puts many of them under /usr/local/include/kissfft.

I suppose in a case like this it would probably be best to place them in a subdir. However this is effectively discussing where you expect reverse dependencies to use, e.g. #include <kiss_fft.h> or #include <kissfft/kiss_fft.h>. Since both .h starts with kissfft then it doesn't really change as they are already namespaced.

However, if you really plan to install tools/*.h as well (do we really need them?), then yes, definitely place that into a subdir. You wouldn't want tools in the global include namespace.

mapreri commented 3 years ago

version numbers?

While we're on the subject, what about versioning the lib.so? Anybody know how to go about that?

I suggest keeping ABI version and soname consistent with Debian (libkissfft-float.so.131) but tag the 131.1 release that can be imported everywhere. My friend and colleague Mattia can add more on it.

Personally, I'm a big, huge fan of semver for versioning libraries, and I'd generally expect to see the version of a library be different from the one of the project it belongs to. I wouldn't find surprising to see libfoo.so.1.2.3 coming from project foo version 3.2.1.

As such, I believe you should separate those concepts, and use the version of the lib.so only to track its ABI, not the API or whatever other thing the project version stands for.

I understand that might be hard to do for some projects, which is why I understand why they stick to one project release == one version == one SONAME/SOVERSION. However people needs to keep in mind that doing that forces reverse dependencies to be rebuilt, which is not always trivial or possible. Tying a library version to its project version is really doable only if the releases are rare enough that doing the required steps of rebuilding reverse dependencies that much often is not too much effort.

The tl;dr of this is: please don't change SOVERSION when you release non-breaking bugfixes :)

basilgello commented 3 years ago

However, if you really plan to install tools/*.h as well (do we really need them?)

Yes we need them because I already made them part of public API :) Next step is to move them from 'tools' to 'include', and I am fine to have '/usr/include/kfc.h' as well as '/usr/include/kissfft/kfc.h'.

I hope KFC (Kentucky Fried Chicken) will not ever make a Debian package dropping its include 'kfc.h' into '/usr/local' :) Otherwise, we should stick to '/usr/local/kissfft' path :)

mborgerding commented 3 years ago

version numbers?

While we're on the subject, what about versioning the lib.so? Anybody know how to go about that?

... Personally, I'm a big, huge fan of semver for versioning libraries, and I'd generally expect to see the version of a library be different from the one of the project it belongs to.

I admit I am blissfully ignorant of the intricacies of versioning. I had to look up "semver". I like the idea of versioning SO/DLLs based on the ABI, not an underlying code snapshot.

I think this would change very slowly indeed for kissfft. The kissfft extern "C" float ABI has not had a minor change since a few functions were added in 2006. The last patch-level bugfix I see is 2009.
(Note: I am excluding the header-only c++ kissfft<T> class from this analysis)

The path of least resistance would be to just decide what today's ABI version is called (1.0.0? 2.0.0?) and hard code that into the cmake/make installation configuration. We can manually change it when the ABI changes(major), functions are added(minor) or bugs are fixed(patch level).

basilgello commented 3 years ago

I hope KFC (Kentucky Fried Chicken) will not ever make a Debian package dropping its include 'kfc.h' into '/usr/local' :) Otherwise, we should stick to '/usr/local/kissfft' path :)

Typo, should be '/usr/include' everywhere here!

24 січня 2021 р. 15:55:10 UTC, Vasyl Gello vasek.gello@gmail.com написав(-ла):

However, if you really plan to install tools/*.h as well (do we really need them?)

Yes we need them because I already made them part of public API :) Next step is to move them from 'tools' to 'include', and I am fine to have '/usr/include/kfc.h' as well as '/usr/include/kissfft/kfc.h'.

I hope KFC (Kentucky Fried Chicken) will not ever make a Debian package dropping its include 'kfc.h' into '/usr/local' :) Otherwise, we should stick to '/usr/local/kissfft' path :)

basilgello commented 3 years ago

The path of least resistance would be to just decide what today's ABI version is called (1.0.0? 2.0.0?) and hard code that into the cmake/make installation configuration. We can manually change it when the ABI changes(major), functions are added(minor) or bugs are fixed(patch level)

Currently I marked ABI as 131, but since we have 3 days to refactor the library within Debian (and it is waaay easier than one might think), nothing prevents us from starting ABI from 1.0.0. Or we can do even less and stick to 131.0.0 - since the very first CMake file and the very last tagged release was 131. If we keep ABI 131, and tag version 131.1.0 on Github Releases, we still get 'libkissfft-float.so.131' and 132 release will only be needed if any API function is removed. Otherwise, adding new APIs or bug fixes will increment minor and patch digits, respectively (131.1.1, 131.1.2, 131.2.0 etc)

jtojnar commented 3 years ago

Typo, should be '/usr/include' everywhere here!

The convention is $(prefix)/include. And prefix should default to /usr/local (which is what people will use when manually building and installing, to distinguish it from installing using a distro package). Linux distros override the prefix to /usr.

If there are multiple headers the convention is to use a subdirectory. It is not necessary to mention that subdirectory in #includes, you can just list Cflags: -I${includedir}/subdir in the .pc file (see https://people.freedesktop.org/~dbn/pkg-config-guide.html or https://linux.die.net/man/1/pkg-config).

mapreri commented 3 years ago

If there are multiple headers the convention is to use a subdirectory. It is not necessary to mention that subdirectory in #includes, you can just list Cflags: -I${includedir}/subdir in the .pc file

oh, right. I honestly didn't realize that kissfft was shipping a .pc file. I suppose so then, rdeps can just go and take the -I from there and you can just dump all headers into a subdir without too much thoughts indeed.

jtojnar commented 3 years ago

Superseded by https://github.com/mborgerding/kissfft/pull/58