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

Add CMake build system #544

Merged
merged 60 commits into from
Mar 19, 2016
Merged

Add CMake build system #544

merged 60 commits into from
Mar 19, 2016

Conversation

Lekensteyn
Copy link
Contributor

This PR adds CMake support as discussed in #448. Advantages over autotools build include:

  • Faster configuration step (see below) and iterative builds (esp. with Ninja)
  • Full out-of-tree build support without cluttering source tree (with files such as configure, Makefile.in, etc.)
  • In theory easier support for Windows (only tested for the library with CUnit tests, not the examples or programs).
  • No static library is built nor installed, reducing the time required for make check (see below for times)

Notes related to CMake:

  • Only numeric versions are supported, hence the version is reported as 1.8.90 instead of 1.9.0-DEV.
  • libev 4.11 (instead of 4.15) seems to work too, the README and CMakeLists.txt file have been updated to reflect this (any reason why it said 4.15 before? Travis uses 4.11...).
  • Warnings are enabled by default (-DENABLE_WERROR=1 will additionally add -Werror). Additionally, -Wunused-macros and -Wstrict-prototypes are correctly detected (autotools fails because of crap tests).
  • The check target is missing if cunit is not availble. Could be changed to a dummy target though.
  • CMAKE_BUILD_TYPE=RelWithDebInfo is set which adds -g -O2 -DNDEBUG (disables assertions too!).

Notable modifications to non-cmake files (git rebase master && git diff --diff-filter=M master..):

  • .gitignore: added cmake stuff
  • .travis.yml: added cmake+gcc/clang besides autotools+gcc/clang
  • **/Makefile.am: added cmake-related files to EXTRA_DIST
  • integration-tests/setenv.in, python/setup.py.in: added cmake-compatible libdir

distcheck is passing with this branch + #542 included (see https://travis-ci.org/Lekensteyn/nghttp2/builds/115968744), cmake can be built from the tarball.


Above build benchmarks were done with this script on 4d801cd, times are based on the mean of the real times, standard deviation is insignificant:

#!/bin/sh
set -u -e
git clean -xfd
autoreconf -vifs
l=logs
mkdir $l
cm='cmake -DCMAKE_C_FLAGS="-g -O2" -DCMAKE_CXX_FLAGS="-g -O2"'
for i in {1..3};do
    r='&& mkdir $_ && cd $_ && time '
    script -c "rm -rf b$r $cm         .. && time make -j10 all check" $l/make-${i}.log
    script -c "rm -rf c$r $cm -GNinja .. && time ninja     all check" $l/ninja-${i}.log
    script -c "rm -rf a$r ../configure   && time make -j10 check" $l/auto-${i}.log
done

times for the check target with cunit installed (n=3):

what mean stdev
ninja-config 7.49 0.020
make-config 8.10 0.045
auto-config 10.05 0.104
ninja-build 27.75 0.274
make-build 30.99 0.032
auto-build 30.59 0.196

FWIW, make/ninja install takes 34ms, 500ms and 991s respectively.

Not working:
 - option(... check)
 - not finished everything (see XXX and FIXME)
 - still halway converting
AC_CHECK_TYPES and AC_C_BIGENDIAN were removed because nothing checks
the resulting macros...
Also remove some headers which were not checked anyway and add macros to
cmakeconfig.h.in (based on the headers list in the CMakeLists.txt file).
Do not use minus sign in macro names.
Must be absolute instead of relative
Remaining work:
 - integrate mruby and neverbleed
 - integrate cunit
Avoids leaking compile flags to dependents.
Might not be the best approach, maybe use thewtex/cython-cmake-example?
Remove build/host/target by a single target system name (CMake is
different for cross-compiling, you are suggested to set
CMAKE_TOOLCHAIN_FILE).

Fix various library variables, remove CFLAGS (INCLUDE_DIRS could be used
instead though, but I consider that minor information that could be
added later if wanted).

Fix various variable names (prefix, boost, etc.).
Add auto-detection to the most important features (app, hpack, etc.).
Move options to a separate file for easier search.

Add cmake-based Libevent, jansson and CUnit search. Move pkg-config
handling for Libev and jemalloc to their cmake files.

Note: duplicates find_package before including CMakeOptions.txt and when
checking for features. Maybe that can be cleaned up later...
Split the nghttp2 library into objects and a shared library from those
objects. This is needed because of symbol visibility. An advantage over
the autotools build is that there are no worries about static versus
static library builds.

Test:

    cmake $srcdir
    make nghttpx-unittest main failmalloc
    make test
Need to add -fPIC to objects that will be put in a shared library.
mruby is always invoked now (mirrors the autotools behavior). It could
be optimized though to only trigger the mruby build when the static
library is missing.

Also fix typo in NGHTTP2_TESTS_DIR macro definition (detected when
invoking the Ninja generator).
FindCython.cmake was taken from
https://github.com/thewtex/cython-cmake-example. The UseCython module
works, but since it is lacking an installation target setup.py will be
used instead.
Auto-detect spdylay availability using CMake, making pkg-config
completely optional.
Some detected libraries were not reported when a feature is disabled.
This change removes unnecessary second find_package calls and sets
HAVE_xxx immediately based on xxx_FOUND.
The COMPILE_LANGUAGE generator expression is only supported since CMake
3.3. Moreover, it does not work with all generators (works with Makefile
and Ninja, but not with Visual Studio).

target_compile_options would only work if a target does not mix C and
C++ sources, since the flags are intended to be set for a specific
language, use set_source_files_properties instead. This approach is also
less repetitive.

Drop the idea of using lists and COMPILE_OPTIONS,
set_source_files_properties only understands COMPILE_FLAGS (a single
string, not a list).
libev 4.11 seems to build fine as demonstrated by Travis with autotools.
The name passed to find_package should match the one that is passed to
find_package_handle_standard_args, otherwise the version matching is not
properly done. The names are based on the FindXXX.cmake filename.

This also simplifies components reporting for Libevent (done by
find_package_handle_standard_args) and results in reporting a library
instead of include directory when found.
Caught with cmake --warn-uninitialized.
Remove generated documentation, python and mruby build artifacts.
Note that this does not work for Ninja, Makefile works fine though.
Adds missing source files and configure.ac changes since
v1.7.0-93-g093eb51.
Update to latest master with appropriate cmake changes at the same time.
Add Nathan's PPA for cmake 3.2.3 (3.0 or newer is required).
NOTE: RelWithDebInfo *disables* assertions by default. To keep
assertions, use CMAKE_BUILD_TYPE=Debug or CMAKE_BUILD_TYPE=None.
@Lekensteyn
Copy link
Contributor Author

@tatsuhiro-t In 9218d5d you added AC_HEADER_ASSERT "for benchmark", what benchmark? That option has as side-effect that the --disable-assert option becomes available.

In this PR, assertions are disabled by default (because CMAKE_BUILD_TYPE=RelWithDebInfo does so). To keep assertions and have debug info, use CMAKE_BUILD_TYPE=Debug (which is different from autotools in the sense that -O2 is not added).

When asserts are disabled, there are some warnings...

lib/nghttp2_session.c: In function ‘nghttp2_session_get_remote_settings’:
lib/nghttp2_session.c:6767:1: warning: control reaches end of non-void function [-Wreturn-type]

src/h2load_http2_session.cc: In member function ‘virtual void h2load::Http2Session::on_connect()’:
src/h2load_http2_session.cc:176:7: warning: variable ‘rv’ set but not used [-Wunused-but-set-variable]

...

Should assertions always be enabled, even for release builds?

Build type-specific flags are added before other flags, reflect this in
the output.
@tatsuhiro-t
Copy link
Member

Thank you for PR, I'll review this tomollow (I have a software release in another project today).

For assertions, I personally think its effect on performance in practice is negligible, so they should be enabled by default to give us useful debugging information. FYI, autotools can use -O2 and assertion enabled at the same time.

@Lekensteyn
Copy link
Contributor Author

I also think that assertions are still useful in release builds, but the default cmake logic is apparently to disable them (that is, when you use -DCMAKE_BUILD_TYPE=Release). If you set -DCMAKE_BUILD_TYPE=None -DCMAKE_C_FLAGS="-g -O2" -DCMAKE_CXX_FLAGS="-g -O2" you get the same result as autotools.

Should I unconditionally enable assertions for various CMAKE_BUILD_TYPE values or add an option for this (-DENABLE_ASSERT=1|0, mirrors --enable-assert/--disable-assert from autotools). I tend to prefer the former option.

@tatsuhiro-t
Copy link
Member

I also prefer the former.

@tatsuhiro-t
Copy link
Member

PR looks good.

Users can disable this by setting -DCMAKE_C_FLAGS=-DNDEBUG or
-DCMAKE_CXX_FLAGS=-DNDEBUG to disable assertions as desired.
Do not add _U_ and NGHTTP2_NORETURN definitions to the command line,
instead add it to config.h. This matches what autotools does.
Matches autotools behavior. The m4_if logic was misread...
Avoid adding a LIBXML2_LIBRARIES-NOTFOUND to the libraries list when it
is not found. Likewise for OpenSSL.
Fix Windows build by defining `ssize_t` when missing and adjusting the
install commands.

Add support for ENABLE_WERROR=1 while at it.

Tested with MSVC 2013 on Windows 7 x64.
failmalloc and main tests succesfully pass the test now.
It was added because the default RelWithDebInfo type disabled
assertions, but now that -DNDEBUG is removed from its flags, it is not
necessary to change the build type anymore.
@Lekensteyn
Copy link
Contributor Author

The assertions are fixed now (well, except on Windows where you have to define DEBUG explicitly for assertions).

Speaking of Windows, the library can now be built on Windows (tested with cmake && cmake --build). This only gives three warnings (vsnprintf usage and missing return because assertions were disabled). The main and failmalloc tests also run. Build log: nghttp2-v1.8.0-141-g08c7656-cmake-win7.txt (have a look, on_stream_close_callback uses uint32_t error_code which should probably be the nghttp2_error_code)

All dependencies are missing on Windows, so I had to hack a bit to get stuff working:

Unfortunately I could not get the app to compile. The headers getopt.h and sys/un.h are missing (via utils.h) and template.h blows up completely.

Nevertheless I think that the cmake build is ready to go, others can add more Windows as desired. NOTE: only a static nghttp2.lib file is built, no dll.

@tatsuhiro-t
Copy link
Member

LGTM. Compiling applications on Windows are known issue, currently they are *nix centric.

@tatsuhiro-t tatsuhiro-t merged commit 08c7656 into nghttp2:master Mar 19, 2016
@tatsuhiro-t
Copy link
Member

Merged now. Thank you for your awesome work!

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.

2 participants