Skip to content

Simplify CMake build using add_subdirectory #207

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

Merged
merged 9 commits into from
May 29, 2016
Merged

Conversation

dean0x7d
Copy link
Member

@dean0x7d dean0x7d commented May 22, 2016

This PR and the following description are a bit long, but please bear with me as I hope this could be a nice improvement for those using the CMake build system.

The proposal greatly simplifies the process of creating extensions using CMake. Instead of the manual process which is currently described in the documentation, the user could simply write:

cmake_minimum_required(VERSION 2.8.12)
project(example)

add_subdirectory(pybind11)
pybind11_add_module(example src/main.cpp)

where pybind11_add_module() is the new function proposed in this PR. These 4 lines are all that is needed to build a pybind11 module. This is similar to the builtin add_library() function. It will create a pybind11 extension module called example from the main.cpp source (but any number of source files may be specified).

A full example is available in this repository. It also shows how the CMake build can be integrated with Python's regular setup.py system.

This PR consists of the following steps:

  1. The first commit replaces CMake's builtin FindPythonLibs with a much more robust one from Continuum Analytics. This address problems with finding some non-system Python installations as reported previously in CMake finding Python library and Python interpreter mismatch during pybind11 build #99. So homebrew python on OS X and some Anaconda installations will be found without requiring any user intervention.

The new FindPythonLibs also identifies the proper library prefix and extensions to use on any platform. This is important because newer versions no longer use just a simple ".so" or ".pyd". Instead, the platform and version are also embedded. For example, for Python 3.5 on Linux the extension is ".cpython-35m-x86_64-linux-gnu.so".
2. Next, a module creation function is added with the signature pybind11_add_module(<name> source1 [source2 ...]). This works as described above in the introduction and example repo.

This requires using target-specific CMake commands, e.g. target_include_directories instead of include_directories. This is considered to be a more modern CMake style and it makes things easier when including one project from another. The required CMake version is bumped from 2.8 to 2.8.12 because of this. As far as I know this should not be a problem for users. The latest version is already at 3.5.
3. The next commit just updates CMake on Travis.
4. Finally, all the CMake code that's related to test/examples is moved into the subdirectory. The tests are then only built if pybind11 is used as the master project. When the project is added using add_sudirectory the test and install targets are not active.

@wjakob
Copy link
Member

wjakob commented May 24, 2016

This looks really interesting -- it will take me a bit to digest all of this, bear with me as well :).

One quick question -- this was not immediately obvious to me -- is pybind11_add_module() somehow exported to the parent scope when included from another project?

What if the outer project changes the CMAKE_CXX_FLAGS after add_subdirectory(pybind11) call. Will those be reflected in pybind11_add_module()

Thanks!

@dean0x7d
Copy link
Member Author

One quick question -- this was not immediately obvious to me -- is pybind11_add_module() somehow exported to the parent scope when included from another project?

Functions and macros have global scope in CMake so they are immediately available to the parent project. Variables have function/directory scope by default, so they are not exported to the parent unless they are marked with PARENT_SCOPE or CACHE.

What if the outer project changes the CMAKE_CXX_FLAGS after add_subdirectory(pybind11) call. Will those be reflected in pybind11_add_module()

Yes, they will. However, since the introduction of the target-specific commands, it is not recommended to use the global CMAKE_CXX_FLAGS any more. The new system works quite nicely:

add_library(mylib SHARED ${libsources})
target_compile_options(mylib PUBLIC -std=c++11)
target_compile_options(mylib PRIVATE -flto)

add_executable(myexe ${exesources})
target_link_libraries(myexe PUBLIC mylib)

In this case, because myexe is linked to mylib, it will inherit the public -std=c++11 flag, but it will not get the private -flto. The same applies to include directories which can also be private or public (and thus inherited). This makes managing multiple build targets much easier since there is no need to worry about the global flags/definitions and when they do or don't apply.

That reminds me: I left a few uses of CMAKE_CXX_FLAGS in the examples subdirectory. If you agree, I can go through that and convert it to the new target-specific commands.

@wjakob
Copy link
Member

wjakob commented May 24, 2016

Wow, that looks like a great improvement to CMAKE_CXX_FLAGS. My only concern is that there are still many machines running some 2.8.x version of CMake -- do they support this syntax?

Is this just for compile flags, or are there similar functions for compile definitions, include paths, and linker flags?

@dean0x7d
Copy link
Member Author

dean0x7d commented May 24, 2016

Version 2.8.12 is the minimum requirement. For example, Ubuntu 14.04 LTS comes with this exact version by default, so that's about the timeframe for other distributions, as far as I'm aware. Since pybind11 is C++11 only, I don't think any users will be running anything too old anyway. But it's pretty trivial to upgrade CMake (their website even has a pre-compiled binary .sh install for Linux). IMHO the nicer CMake workflow is well worth it.

Everything works like this: target_include_directories, target_compile_definitions and target_link_libraries. It's not really obvious, but target_link_libraries is the preferred way of passing linker flags.


project(pybind11)

option(PYBIND11_INSTALL "Install pybind11 header files?" ON)
# Check if pybind11 is being used directly or via add_subdirectory
set(MASTER_PROJECT OFF)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prefix MASTER_PROJECT with PYBIND11_

@wjakob
Copy link
Member

wjakob commented May 26, 2016

Hi,

I went through your changes and just had two minor comments on the changes. Also, one remaining issue is that the current documentation (docs/compiling.rst) does not make much more sense with this change. Could you please update the part on CMake following your approach here?

Thanks,
Wenzel

@dean0x7d
Copy link
Member Author

Sure, I'll update the CMake section in the docs.

@dean0x7d
Copy link
Member Author

I made changes based on the comments and updated the documentation. For the documentation, I included a link to the CMake example repository that I set up. I hope that was not too presumptuous of me.

Let me know if everything is OK. I can rebase and squash the smaller commits if needed.

@wjakob
Copy link
Member

wjakob commented May 26, 2016

No need to squash. I would add a recommendation to also add the following optional flags (copied from example/CmakeLists.txt) -- LTO and stripping the binary both makes a big difference when linking together an extension module from many files (size reduction-wise -- nobody wants to redistribute bloated .so files for release builds)

if(CMAKE_CXX_COMPILER_ID MATCHES "Clang" OR CMAKE_CXX_COMPILER_ID MATCHES "GNU" OR CMAKE_CXX_COMPILER_ID MATCHES "Intel")
  # Enable link time optimization and set the default symbol
  # visibility to hidden (very important to obtain small binaries)
  if(NOT ${U_CMAKE_BUILD_TYPE} MATCHES DEBUG)
    # Check for Link Time Optimization support
    # (GCC/Clang)
    CHECK_CXX_COMPILER_FLAG("-flto" HAS_LTO_FLAG)
    if(HAS_LTO_FLAG)
      set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -flto")
    endif()

    # Intel equivalent to LTO is called IPO
    if(CMAKE_CXX_COMPILER_ID MATCHES "Intel")
      CHECK_CXX_COMPILER_FLAG("-ipo" HAS_IPO_FLAG)
      if(HAS_IPO_FLAG)
        set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -ipo")
      endif()
    endif()
  endif()
endif()

if(WIN32)
  if(MSVC)
    # /MP enables multithreaded builds (relevant when there are many files), /bigobj is
    # needed for bigger binding projects due to the limit to 64k addressable sections
    set_property(TARGET example APPEND PROPERTY COMPILE_OPTIONS /MP /bigobj)

    # Enforce size-based optimization and link time code generation on MSVC
    # (~30% smaller binaries in experiments); do nothing in debug mode.
    set_property(TARGET example APPEND PROPERTY COMPILE_OPTIONS
      "$<$<CONFIG:Release>:/Os>" "$<$<CONFIG:Release>:/GL>"
      "$<$<CONFIG:MinSizeRel>:/Os>" "$<$<CONFIG:MinSizeRel>:/GL>"
      "$<$<CONFIG:RelWithDebInfo>:/Os>" "$<$<CONFIG:RelWithDebInfo>:/GL>"
    )
    set_property(TARGET example APPEND_STRING PROPERTY LINK_FLAGS_RELEASE "/LTCG ")
    set_property(TARGET example APPEND_STRING PROPERTY LINK_FLAGS_MINSIZEREL "/LTCG ")
    set_property(TARGET example APPEND_STRING PROPERTY LINK_FLAGS_RELWITHDEBINFO "/LTCG ")
  endif()
elseif(UNIX)
  # Optimize for a small binary size
  if(NOT ${U_CMAKE_BUILD_TYPE} MATCHES DEBUG)
    set_target_properties(example PROPERTIES COMPILE_FLAGS "-Os")
  endif()

  # Strip unnecessary sections of the binary on Linux/Mac OS
  if(APPLE)
    if(NOT ${U_CMAKE_BUILD_TYPE} MATCHES DEBUG)
      add_custom_command(TARGET example POST_BUILD COMMAND strip -u -r $<TARGET_FILE:example>)
    endif()
  else()
    if(NOT ${U_CMAKE_BUILD_TYPE} MATCHES DEBUG)
      add_custom_command(TARGET example POST_BUILD COMMAND strip $<TARGET_FILE:example>)
    endif()
  endif()
endif()

By the way, the /bigobj flag is still in the above code, and it's also in example/CMakeLists.txt. I think this also qualifies as one of those flags that should be moved into the pybind11_add_module function. Without that, the compiler will often just crash because it runs out of COMDAT sections.

Finally, I would propose renaming pybind11_add_module to add_pybind11_module to be in line with CMake naming conventions (add_library, add_executable, etc.)

@dean0x7d
Copy link
Member Author

For the naming, I agree that add_pybind11_module would make more sense, but the prefix naming looks to be a well established convention in CMake, both for variables and functions. For example, the CMake CUDA module has cuda_add_executable and cuda_add_library.

I added LTO, strip and bigobj to the function. I was a bit hesitant to include anything more than the minimum flags required for the extension to work correctly, but with lots of template-driven code generation that happens here, it makes sense to include those flags.

One extra note: I completely removed the manual configuration of the -Os flag. Setting MinSizeRel takes care of it automatically.

@dean0x7d
Copy link
Member Author

Just one last addition: the PYBIND11_CPP_STANDARD variable allows the standard to be set explicitly. Someone might want only -std=c++11 or maybe something newer like -std=c++1z.

@wjakob wjakob merged commit 1503d2f into pybind:master May 29, 2016
@wjakob wjakob mentioned this pull request May 29, 2016
@wjakob
Copy link
Member

wjakob commented May 29, 2016

This was seriously good stuff -- thank you for all of your work!

@dean0x7d
Copy link
Member Author

I'm glad I could contribute. Thank you for the awesome library.

@dean0x7d dean0x7d deleted the cmake branch May 29, 2016 16:08
@wjakob
Copy link
Member

wjakob commented Jun 3, 2016

@dean0x7d
Copy link
Member Author

dean0x7d commented Jun 3, 2016

Hm, yes. It's now impossible for it to mismatch the interpreter and library, because the library information is gathered directly from the interpreter. So that part can be safely deleted.

It's still possible for it to completely miss the interpreter. In that case it's enough to set just the PYTHON_EXECUTABLE variable. It should be a rare case, but perhaps it is worth mentioning in the FAQ in place of the current entry.

pramodk added a commit to BlueBrain/nrn that referenced this pull request Jul 21, 2019
nrnhines pushed a commit to neuronsimulator/nrn that referenced this pull request Jul 23, 2019
* Started top level refactoring

* Top level CMake 2

* Top level CMake refactor part 3

* Top level refactoring 4

* Top level refactoring 5

* Add build status after cmake stage

* CMake modules refactoring

* Avoid python2 library detection when default python is 3

I was getting:
-- Python        | 1
--   EXE         | /usr/local/Cellar/python/3.7.2_2/bin/python
--   LIB         | /usr/lib/libpython2.7.dylib

Prefer version detected as interpreter

* Bug fix for python library detection, use pybind11 module
which has worked very well.

See: pybind/pybind11#207

* WIP Refactoring : cmake_config/CMakeLists.txt

* Refactoring of cmake_config/CMakeLists.txt part 2

* Big refactoring of src/nrniv/CMakeLists.txt

* Refactoring of src/nrnpython/CMakeLists.txt

* Bug fixes for ubuntu
pramodk added a commit to neuronsimulator/nrn that referenced this pull request Dec 2, 2019
* Cmake -DENABLE_INTERVIEWS=OFF works
* CMake build system refactoring (#247)
  - Top level refactoring 5
  - Add build status after cmake stage
  - CMake modules refactoring
  - Avoid python2 library detection when default python is 3
     - Python      | 1
     - EXE         | /usr/local/Cellar/python/3.7.2_2/bin/python
     - LIB         | /usr/lib/libpython2.7.dylib
* Prefer version detected as interpreter
* Bug fix for python library detection, use pybind11 module
  which has worked very well.
  - See: pybind/pybind11#207
* Rpath issue and nrnivmodl fixes (#249)
* Remove interview target link while building special
* Fix rpath issue with shared libraries
    - see https://stackoverflow.com/questions/32283062/compiling-c-shared-library-with-distutils-setup-py-when-the-library-depends-on
    - include IV directories
* Fix link error with dynamic mpi
* Fix missing darwin definition: (#253)
  This was causing following error:
    testNumpyInteraction (neuron.tests.test_vector.VectorTestCase)
    Testing numpy.array <=> hoc.Vector interaction ... build_cmake.sh: line 16: 11620 Segmentation fault: 11  $python -c 'from neuron import test; test()'
* Set missing IV_LIBDIR (#254)
* Add missing rpath for interviews lib directory
* Small changes while testing on Ubuntu
  - print python include directory (which could be missing)
  - check python dir exist to avoid comple time error
* Fix missing linking with -lpthread and -ldl with dynamic mpi and python on linux
* Fix issue with python dynamic support is enabled
  - I see below error when trying to use python dynamic build:
    ```
    nrn/install-dir/x86_64/bin/nrniv -python -pyexe python3.7 -c 'import neuron ; neuron.test() ;'
    NEURON -- VERSION 7.7.1-24-g4524ea3c master (4524ea3) 2019-07-31
    Duke, Yale, and the BlueBrain Project -- Copyright 1984-2018
    See http://neuron.yale.edu/neuron/credits

    bash: nrnpyenv.sh: No such file or directory
    Python not available
    ```
  - This is because popen fails for `bash nrnpyenv.sh` fails.
* Install nrnpyenv.sh to install-prefix/bin

Co-authored-by: Pramod Kumbhar <[email protected]>
Co-authored-by: Michael Hines <[email protected]>
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