Add CMake build system (based on previous work from @mdsitton and @jcelerier) and CI #24

Bobo1239 · 2017-11-26T23:26:43Z

Hi @erikd,
thanks for creating this library. I wanted to create some Rust bindings and to simplify cross-platform compilation I continued the previous CMake attempts from @mdsitton and @jcelerier. I've also tried to setup some CI (Travis for Linux; Appveyor for Windows). Admittedly I've not really done much C development till now so please feel free to point out any problems with this PR.

https://travis-ci.org/Bobo1239/libsamplerate
https://ci.appveyor.com/project/Bobo1239/libsamplerate

erikd · 2017-11-28T08:37:14Z

This is a pretty large PR. Probably won't get time to have a proper look at it until the weekend.

jcelerier · 2017-11-28T08:58:58Z

hey, sorry, I had entirely forgot my PR. this looks pretty nice and complete ! But are you sure you want to support CMake versions that didn't even have PUBLIC / PRIVATE ? Are there still widely-used systems with such old version (that couldn't just install CMake's statically-built latest version) ?

Bobo1239 · 2017-11-28T10:49:10Z

@erikd No worries, take your time. :)
@jcelerier Tbh I was just trying to get everything here to become green (as they are now) and some of those platforms (they are Docker images) have CMake 2.8. I figured we aren't really using any new CMake features so it wouldn't hurt to support an older verson. But as I said in my opening post I don't have an overview over this ecosystem so you guys have to make the call whether it's worth it to support old CMake versions.

erikd · 2017-11-28T20:11:58Z

libsndfile builds on travis with CMake min version of 3.1.3. Maybe figure out what that is doing.

Bobo1239 · 2017-11-28T22:32:14Z

Those builds aren't normal Travis instances; they are cross compiling and testing on many different Docker images (some of them using qemu internally?) and aren't maintained by me. (They are from cross which is ""Zero setup" cross compilation and "cross testing" of Rust crates"). If you guys don't think it's worth supporting such an old CMake version I have no problems reverting the last few changes. I would then just drop those few Travis instances or try to upgrade CMake in those Docker images.

erikd · 2017-11-29T08:47:20Z

Yeah, I don't think its worth supporting any CMake version older than the ones libsndfile uses.

jcelerier · 2017-11-29T09:09:47Z

that's interesting: these docker images have rust which is very recent, but don't have five-years old versions of CMake :p are they hosted somewhere on github ? it could be worth it to send PRs to them so that everyone doing such a work can at least leverage recent versions.

Bobo1239 · 2017-11-29T15:29:28Z

Reverted the last changes.
The docker images are here. They are using different Ubuntu base images (ranging from 12.04 to 16.04) because of cross-rs/cross#85. (Basically they're using the oldest possible glibc to maximize support for the produced binaries.)
edit: I'll just update CMake on those Docker images. There's already a script to install a specific CMake version which is already used in some images and most of them are based on Ubuntu 16.04 (CMake 3.5.1) anyways.

tresf · 2017-12-15T15:27:08Z

@Bobo1239 do you offer any build artifacts for libsamplerate? If not, would the project entertain an automatic upload (e.g. github releases, etc) to complement the cmake support?

I'm most interested in the win32 artifacts for multi-platform. The other OSs (unix/linux/macos) have methods to fetch libsamplerate. CMake drastically simplifies the MSVC++ setup and I'd like to add the 32-bit and 64-bit artifacts (plus headers) into the scoop.sh package manager.

tresf · 2017-12-15T16:54:22Z

CMakeLists.txt

+set(HAVE_ALSA ${ALSA_FOUND})
+
+find_package(Sndfile)
+set(HAVE_SNDFILE ${SNDFILE_FOUND})


@Bobo1239 cmake does not seem to provide FindSndfile.cmake or FindFFTW.cmake on some systems which means if they're in a non-standard location, the linking and including won't work as it would in a vanilla POSIX environment.

Would you consider adding an additional pkg-config dependency on this? Then you could do the following:

diff --git a/CMakeLists.txt b/CMakeLists.txt index 394de75..1b85d17 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -9,6 +9,7 @@ include(CheckIncludeFiles) include(CheckSymbolExists) include(CheckTypeSize) include(ClipMode) +include(FindPkgConfig) set(SAMPLERATE_SRC ${PROJECT_SOURCE_DIR}/src/samplerate.c @@ -53,12 +54,24 @@ clip_mode() find_package(ALSA) set(HAVE_ALSA ${ALSA_FOUND}) +if(ALSA_FOUND) + include_directories("${ALSA_INCLUDE_DIRS}") + link_directories("${ALSA_LIBRARY_DIRS}") +endif() -find_package(Sndfile) +pkg_check_modules(Sndfile sndfile>=1.0.0) set(HAVE_SNDFILE ${SNDFILE_FOUND}) +if(SNDFILE_FOUND) + include_directories("${SNDFILE_INCLUDE_DIRS}") + link_directories("${SNDFILE_LIBRARY_DIRS}") +endif() -find_package(FFTW) +pkg_check_modules(FFTW3 fftw3>=3.0.0) set(HAVE_FFTW3 ${FFTW_FOUND}) +if(FFTW3_FOUND) + include_directories("${FFTW3_INCLUDE_DIRS}") + link_directories("${FFTW3_LIBRARY_DIRS}") +endif() configure_file(${PROJECT_SOURCE_DIR}/config.h.in ${CMAKE_CURRENT_BINARY_DIR}/config.h)

FindSndfile.cmake and FindFFTW.cmake are the cmake directory which should be loaded by set(CMAKE_MODULE_PATH ${CMAKE_MODULE_PATH} ${PROJECT_SOURCE_DIR}/cmake) at the top. (ClipMode.cmake is also affected by this.)

Regarding pkg-config: What do @erikd and @jcelerier think? Should I switch from the two cmake modules to pkg-config? I really don't know what is better supported across the board.

I really don't know what is better supported across the board.

To give a bit more background information on this:

Ubuntu 14.04, default for Travis-CI:

locate -i FindALSA.cmake (found) /usr/share/cmake-3.5/Modules/FindALSA.cmake

locate -i FindFFTW (not found)

locate -i FindSndfile (not found)

What this means is if a project requires any of the variables expected to be available for includes or libraries, they won't be available. Msys2 re-uses the POSIX-style paths so it hides this bug as the default include and lib paths work out of the box.

In the case of a build system that has CMAKE_PREFIX_PATH setup to search for these libraries, it'll find that they exist, but not know how to find the headers or the libraries without the appropriate .cmake file if they're installed anywhere except where a Unix environment would expect.

Case and point is that if you were to download the libsndfile from the website, it offers a libsndfile.pc file, not a FindSndfile.cmake file.

@Bobo1239 On second thought, perhaps this is a problem with my environment. CMAKE_PREFIX_PATH clearly states that it searches the same paths as pkg-config. Here's an excerpt from the website:

[CMAKE_PREFIX_PATH] contains the “base” directories, the FIND_XXX() commands append appropriate subdirectories to the base directories. So FIND_PROGRAM() adds /bin to each of the directories in the path, FIND_LIBRARY() appends /lib to each of the directories, and FIND_PATH() and FIND_FILE() append /include .

I will investigate this some more on my end as to why the paths are found, but the includes and libs are not. If working properly, the existing code should work just fine with cmake.

I think you may have misunderstood me. FindFFTW and FindSndFile are distributed with the source (in the cmake directory) in the repo and aren't required from the environment. And they should get loaded because we're extending CMAKE_MODULE_PATH to include the cmake directory.
edit: Didn't see your response before I submitted this.

@Bobo1239 you are correct, I missed that they were distributed with the project. I think I found the culprit: https://github.com/erikd/libsamplerate/pull/24/files#r157310099. Your original technique should be just fine but I recommend making the variables consistently named. Also, include_directories(...) and link_directories(...) will still be required for some environments. That part will still need to be (fixed to use the right variables and) added as a safeguard, even if redundant on some systems.

I looked at some more FindXXX.cmake modules and they all appear to follow slightly different conventions, so it's hard to argue for consistency. Working with what we have now... Here's what works out of the box using this PR without adding pkg-config:

diff --git a/CMakeLists.txt b/CMakeLists.txt index 394de75..a2aad7f 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -53,11 +53,23 @@ clip_mode() find_package(ALSA) set(HAVE_ALSA ${ALSA_FOUND}) +if(ALSA_FOUND) + include_directories("${ALSA_INCLUDE_DIR}") + link_directories("${ALSA_LIBRARY}/../") +endif() find_package(Sndfile) set(HAVE_SNDFILE ${SNDFILE_FOUND}) +if(SNDFILE_FOUND) + include_directories("${SNDFILE_INCLUDE_DIR}") + link_directories("${SNDFILE_LIBRARY}/../") +endif() find_package(FFTW) +if(FFTW_FOUND) + include_directories("${FFTW_INCLUDES}") + link_directories("${FFTW_LIBRARIES}/../") +endif() set(HAVE_FFTW3 ${FFTW_FOUND}) configure_file(${PROJECT_SOURCE_DIR}/config.h.in ${CMAKE_CURRENT_BINARY_DIR}/config.h)

Bobo1239 · 2017-12-15T20:52:41Z

@tresf I'll try to look into AppVeyor build artifacts this weekend.

tresf · 2017-12-15T22:04:50Z

cmake/FindFFTW.cmake

@@ -0,0 +1,25 @@
+# From: https://github.com/wjakob/layerlab/blob/master/cmake/FindFFTW.cmake
+# FIXME: License concerns?


The license is BSD 2-clause "Simplified" License. (found it at layerlab) This and the BSD 3-clause are GPL compatible and very common for cmake scripts.

I'd be fine with either a BSD or BSD 2-clause license for the CMake files.

tresf · 2017-12-15T22:11:26Z

cmake/FindFFTW.cmake

+  set (FFTW_FIND_QUIETLY TRUE)
+endif (FFTW_INCLUDES)
+
+find_path (FFTW_INCLUDES fftw3.h)


Ok... I think I found the problem. I had expected this to be FFTW_INCLUDE_DIRS and FFTW_LIBRARY_DIRS. ~~See FindSndfile.cmake as an example~~ Edit: No, that's slightly off as well. This is definitely why pkg-config worked and find_package failed. pkg-config uses more predictable names across the board; cmake you can provide your own. I can change my code to accommodate.

tresf · 2017-12-16T18:55:23Z

@Bobo1239 good call on target_link_libraries. a1ba332 now fixes building on non-posix environments. Thanks!

Bobo1239 · 2017-12-17T05:21:52Z

@tresf Would this be enough for your use case?

tresf · 2017-12-17T14:55:03Z

@Bobo1239 I will also be needing the header file and the pkg-config (.pc) file. libsndfile is a good example and here's how I've written the release into the scoop package manager.

Also, long-term, if there are no objections, it would be nice to have the file hosted at an unauthenticated, tagged location such as the github release area, if @erikd would entertain this. AppVeyor is able to do this.

Bobo1239 · 2017-12-17T15:42:06Z

@tresf I guess it would make sense to setup the install target in CMake so we get that kind of structure for all platforms (also we're currently not generating a .pc file at all I think?)
I don't know when I'll have the time to try that out. If you need it rather sooner than later we can maybe try to land this PR first and let somebody else do that?

tresf · 2017-12-17T15:50:42Z

@Bobo1239 AFAIK .pc file should look like this:

prefix=
exec_prefix=${prefix}
libdir=${exec_prefix}/lib
includedir=${prefix}/include

Name: samplerate
Description: An audio Sample Rate Conversion library
Requires: 
Version: 0.1.8
Libs: -L${libdir} -lsamplerate
Libs.private: Ext/libsndfile.a
Cflags: -I${includedir}

CMake can configure this file like so (sample, I haven't tested it for syntax errors)

SET(prefix "")
SET(exe_prefix "\\${prefix}")
SET(libdir "\\${exec_prefix}/lib")
SET(includedir "\\${prefix}/include")
SET(VERSION "${PROJECT_VERSION}")
SET(LIBS "Ext/libsndfile.a")
CONFIGURE_FILE(samplerate.pc.in samplerate.pc @ONLY)

tresf · 2017-12-17T15:53:54Z

@tresf I guess it would make sense to setup the install target in CMake so we get that kind of structure for all platforms

I agree, although I can rearrange the files in the packaging utility if needed, that's what we have to do with fftw's for example. Getting the output files is enough to start offering this package to win32 developers.

Bobo1239 · 2017-12-18T02:14:36Z

@tresf https://ci.appveyor.com/project/Bobo1239/libsamplerate/build/job/psucvyvat0tv6vye/artifacts Does this work for you?

tresf · 2017-12-19T20:14:40Z

@Bobo1239 Yes, it works quite well actually. :)

  -- Checking for module 'samplerate>=0.1.8'
+ --   Found samplerate, version 0.1.9

A working scoop.sh bucket is here.

Bobo1239 · 2018-01-06T01:48:51Z

@erikd Is there anything I should do here?

erikd · 2018-01-06T09:11:18Z

tests/util.c

-	if (file == NULL)
-		return name ;
-
+#if defined (__linux__) || defined (__APPLE__) || defined (__FreeBSD__)


Something's not right there. Why has that #if been removed?

The problem here is the pclose call afterwards which fails to link on Windows. An alternative would be to replace is_pipe with #define IS_PIPE (+ #undef IS_PIPE at the end) and

if (is_pipe) pclose (file) ; else fclose (file) ;

with

#ifdef IS_PIPE pclose (file) ; #else fclose (file) ; #endif

Do you want me to do that?

What problem was this change trying to address?

Native Windows (MSVC) doesn't provide pclose (though apparently it has _pclose) so it must be removed by the preprocessor.

Ok, how about you just sqash this PR down to a single commit and I'll sort it out,

erikd · 2018-01-06T09:13:27Z

Would you be fix the #if issue and then squash that down to a single commit crediting the other two contributors and giving a good overview commit message?

This commit adds a new CMake-based build system. The build time configuration should match the one by autoconf. Then this build system gets tested on Travis (Linux) and AppVeyor (Windows; MSVC and MinGW). Additionally on AppVeyor, a build artifact is produced for easier distribution to Windows. Some minor changes are introduced to fix compilation on Windows. This commit is based on previous attempts at a CMake-based build by Jean-Michaël Celerier (jcelerier) and Matthew Sitton (mdsitton).

Bobo1239 · 2018-01-24T03:49:39Z

Sorry for the delay. Finally got around to do this. (Was busy with university...)
I'm not sure if this qualifies as a "good overview commit message" so tell if there's anything you wish me to add. Besides a rebase onto master I haven't done any changes to the code.
In case you're merging this: don't forget to enable AppVeyor for the repo (and PRs?).

erikd · 2018-02-03T05:42:30Z

Thank you all for your contribution!

tresf reviewed Dec 15, 2017

View reviewed changes

Bobo1239 force-pushed the master branch 3 times, most recently from 36ab528 to 395e851 Compare December 17, 2017 05:05

Bobo1239 force-pushed the master branch 6 times, most recently from bf22644 to 2274269 Compare December 18, 2017 01:55

tresf mentioned this pull request Dec 19, 2017

MSVC support LMMS/lmms#3999

Open

15 tasks

erikd reviewed Jan 6, 2018

View reviewed changes

Bobo1239 force-pushed the master branch from 2274269 to 4785ec6 Compare January 24, 2018 03:44

erikd merged commit 73cb39b into libsndfile:master Feb 3, 2018

This was referenced Apr 5, 2018

More cmake work based on jcelerier's pull request. #8

Closed

Provide minimal CMakeLists #3

Closed

		@@ -0,0 +1,25 @@
		# From: https://github.com/wjakob/layerlab/blob/master/cmake/FindFFTW.cmake
		# FIXME: License concerns?

Add CMake build system (based on previous work from @mdsitton and @jcelerier) and CI #24

Add CMake build system (based on previous work from @mdsitton and @jcelerier) and CI #24

Conversation

Bobo1239 commented Nov 26, 2017 • edited Loading

erikd commented Nov 28, 2017

jcelerier commented Nov 28, 2017

Bobo1239 commented Nov 28, 2017 • edited Loading

erikd commented Nov 28, 2017

Bobo1239 commented Nov 28, 2017

erikd commented Nov 29, 2017

jcelerier commented Nov 29, 2017

Bobo1239 commented Nov 29, 2017 • edited Loading

tresf commented Dec 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tresf Dec 15, 2017 • edited Loading

Choose a reason for hiding this comment

tresf Dec 15, 2017 • edited Loading

Choose a reason for hiding this comment

Bobo1239 Dec 15, 2017 • edited Loading

Choose a reason for hiding this comment

tresf Dec 15, 2017 • edited Loading

Choose a reason for hiding this comment

tresf Dec 16, 2017 • edited Loading

Choose a reason for hiding this comment

Bobo1239 commented Dec 15, 2017

tresf Dec 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tresf Dec 15, 2017 • edited Loading

Choose a reason for hiding this comment

tresf commented Dec 16, 2017

Bobo1239 commented Dec 17, 2017

tresf commented Dec 17, 2017 • edited Loading

Bobo1239 commented Dec 17, 2017

tresf commented Dec 17, 2017

tresf commented Dec 17, 2017

Bobo1239 commented Dec 18, 2017 • edited Loading

tresf commented Dec 19, 2017

Bobo1239 commented Jan 6, 2018

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

erikd commented Jan 6, 2018

Bobo1239 commented Jan 24, 2018

erikd commented Feb 3, 2018

Bobo1239 commented Nov 26, 2017 •

edited

Loading

Bobo1239 commented Nov 28, 2017 •

edited

Loading

Bobo1239 commented Nov 29, 2017 •

edited

Loading

tresf commented Dec 15, 2017 •

edited

Loading

tresf Dec 15, 2017 •

edited

Loading

tresf Dec 15, 2017 •

edited

Loading

Bobo1239 Dec 15, 2017 •

edited

Loading

tresf Dec 15, 2017 •

edited

Loading

tresf Dec 16, 2017 •

edited

Loading

tresf Dec 15, 2017 •

edited

Loading

tresf Dec 15, 2017 •

edited

Loading

tresf commented Dec 17, 2017 •

edited

Loading

Bobo1239 commented Dec 18, 2017 •

edited

Loading