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

Install header files #844

Merged
merged 6 commits into from Nov 28, 2017
Merged

Install header files #844

merged 6 commits into from Nov 28, 2017

Conversation

@tammoippen
Copy link
Contributor

@tammoippen tammoippen commented Oct 24, 2017

This PR make nest install its header files into ${CMAKE_INSTALL_PREFIX}/includes/nest. This allows the development of user modules, without having the nest sources around (cf. email on NEST_USER: [NEST_USER] NEST Homebrew formula: copy headers to install directory).

For this purpose I modified:

  • the output of nest-config --includes via the ALL_INCLUDES_tmp in the base CMakeList.txt
  • added FILTER_HEADERS cmake function that gets all headers from the XXX_sources variables
  • added the install( FILE .... ) instructions for the headers
  • added missing header files (genericmodel_impl.h) to the sources
  • adjusted examples/MyModule/CMakeLists.txt, such that installations into none-default locations of user modules is more explicit; with further instructions to make nest aware of them (see NEST_MODULE_PATH and SLI_PATH)

With the headers installed and travis-ci installation into results (within the source path) the regression test ticket-659-copyright.py failed for some installed headers. In the source, they are explicitly ignored (cf. ticket-659-copyright.py:52ff), hence I ignored the results directory as well.

I tested with cmake 2.8.12, 3.5.1 (ubuntu 16.04) and 3.9.4 (OS X). On the ubuntu system I encountered unrelated compilation issues with gsl and fixed them with 2411f5e. I also tested building and loading MyModule installed into a custom location.

@tclose
Copy link

@tclose tclose commented Nov 6, 2017

Thanks Tammo, the changes work great.

One small, tenuously related issue that might be able to be bundled together with these changes is that when building NEST inside a Python 3 virtual env, the help files don't build because they aren't Python 3 compatible. Do you know where in the make files this is specified and whether the command could be explicitly changed to call python2 instead of python

@tammoippen
Copy link
Contributor Author

@tammoippen tammoippen commented Nov 7, 2017

Hi @tclose ,

Have a look at https://github.com/nest/nest-simulator/blob/master/doc/CMakeLists.txt#L26 .
Here one should require python 2 version, like find_package( PythonInterp 2.7 REQUIRED ).
BUT one should also check, that this will not mess up the nest installation in a mixed python 2/3 environment, since the doc sub dir is called first... the better solution would be to make the doc-generation python 2 and 3 compatible. I think this should definitely be a separate PR. This is also an interesting use case for #853 .

Copy link
Member

@Silmathoron Silmathoron left a comment

Nice work, I made a few comments and have a few questions.

Regarding the python 2/3 problem, I think that another PR is indeed necessary to make the files Py3 compatible.

"${PROJECT_SOURCE_DIR}/topology"
"${PROJECT_SOURCE_DIR}/nestkernel"
"${PROJECT_SOURCE_DIR}/nest"
"${CMAKE_INSTALL_FULL_INCLUDEDIR}/nest"

This comment has been minimized.

@Silmathoron

Silmathoron Nov 7, 2017
Member

Why remove these? Shouldn't a variable starting with ALL_INCLUDES indeed contain all include folders? ^^

This comment has been minimized.

@tammoippen

tammoippen Nov 7, 2017
Author Contributor

The ALL_INCLUDES variable is only used in nest-config. Here we expect to get locations of installed header files. The ${PROJECT_SOURCE_DIR}/... are only available, as long as the NEST sources are there, but (here) we install all headers to "${CMAKE_INSTALL_FULL_INCLUDEDIR}/nest", hence remove all source related paths and have only the install path.

set( CMAKE_INSTALL_LIBDIR ${NEST_LIBDIR}/nest CACHE STRING "object code libraries (lib/nest or lib64/nest or lib/<multiarch-tuple>/nest on Debian)" FORCE )
set( CMAKE_INSTALL_DOCDIR ${NEST_INSTALL_DOCDIR} CACHE STRING "documentation root (DATAROOTDIR/doc/nest)" FORCE )
set( CMAKE_INSTALL_DATADIR ${NEST_INSTALL_DATADIR} CACHE STRING "read-only architecture-independent data (DATAROOTDIR/nest)" FORCE )
if ( "${CMAKE_INSTALL_PREFIX}" STREQUAL "/usr/local" )

This comment has been minimized.

@Silmathoron

Silmathoron Nov 7, 2017
Member

Comments and spacing look weird to me, could you change to

# Use the NEST_INSTALL_* variables as CMAKE_INSTALL_*, if not set explicitly.
if ( "${CMAKE_INSTALL_PREFIX}" STREQUAL "/usr/local" )
  set( CMAKE_INSTALL_PREFIX "${NEST_INSTALL_PREFIX}" CACHE STRING "Install path prefix, prepended onto install directories." FORCE )
  set( CMAKE_INSTALL_LIBDIR ${NEST_LIBDIR}/nest CACHE STRING "object code libraries (lib/nest or lib64/nest or lib/<multiarch-tuple>/nest on Debian)" FORCE )
  set( CMAKE_INSTALL_DOCDIR ${NEST_INSTALL_DOCDIR} CACHE STRING "documentation root (DATAROOTDIR/doc/nest)" FORCE )
  set( CMAKE_INSTALL_DATADIR ${NEST_INSTALL_DATADIR} CACHE STRING "read-only architecture-independent data (DATAROOTDIR/nest)" FORCE )
endif ()

?

I am also a little skeptical about what happens if "${CMAKE_INSTALL_PREFIX}" is not "/usr/local"... how do we know all other CMAKE_INSTALL_* variables are correct? Shouldn't we check all?

This comment has been minimized.

@tammoippen

tammoippen Nov 7, 2017
Author Contributor

... "${CMAKE_INSTALL_PREFIX}" is not "/usr/local" ...

When CMAKE_INSTALL_PREFIX is not set with cmake -DCMAKE_INSTALL_PREFIX=... then it is set to /usr/local. This is the same reasoning we apply to check if nest will be installed to the default location: https://github.com/nest/nest-simulator/blob/master/CMakeLists.txt#L97 There was also an issue discussing this behaviour in NEST, but i cannot find it anymore ...

The other CMAKE_INSTALL_* are set with include( GNUInstallDirs ) later, see here

I will apply the requested changes, see faa528a .

# links to the help files.
add_custom_command( TARGET generate_help POST_BUILD
COMMAND python -B generate_help.py "${PROJECT_SOURCE_DIR}" "${PROJECT_BINARY_DIR}"
COMMAND python -B generate_helpindex.py "${PROJECT_BINARY_DIR}/doc"
WORKING_DIRECTORY "${CMAKE_INSTALL_PREFIX}/${NEST_INSTALL_DATADIR}/help_generator"
WORKING_DIRECTORY "${NEST_INSTALL_PREFIX}/${NEST_INSTALL_DATADIR}/help_generator"

This comment has been minimized.

@Silmathoron

Silmathoron Nov 7, 2017
Member

I'm making a note for myself that this is indeed correct AND necessary so that I do not forget later ^^"

@@ -96,7 +96,7 @@ target_link_libraries( sli_readline sli_lib nestutil ${READLINE_LIBRARIES} )

# add the executable
add_executable( sli puresli.cc )
target_link_libraries( sli sli_lib sli_readline )
target_link_libraries( sli sli_lib sli_readline ${GSL_LIBRARIES} )

This comment has been minimized.

@Silmathoron

Silmathoron Nov 7, 2017
Member

why is this addition necessary?

This comment has been minimized.

@tammoippen

tammoippen Nov 7, 2017
Author Contributor

This is related to:

I tested with cmake 2.8.12, 3.5.1 (ubuntu 16.04) and 3.9.4 (OS X). On the ubuntu system I encountered unrelated compilation issues with gsl and fixed them with 2411f5e. I also tested building and loading MyModule installed into a custom location.

Apparently, cmake 2.8.12 cannot do the transitive library discovery:

nestutil requires gsl
sli requires nestutil, hence sli requires gsl

and does not put gsl into linkage with sli...

Copy link
Member

@Silmathoron Silmathoron left a comment

I am convinced 👍

@tclose
Copy link

@tclose tclose commented Nov 7, 2017

Okay, no worries. A separate PR for building the help pages makes sense. Although I just managed to build NEST in a Python 3 virtualenv on a different machine so I am wondering whether the error I was seeing was due to something strange in my setup (it looked very much like a Python 2/3 error though). Checking that the build is Python 3 compatible is probably a good idea in any case though.

@tammoippen tammoippen force-pushed the tammoippen:includes branch from faa528a to 28a72fb Nov 8, 2017
tammoippen added 6 commits Oct 24, 2017
Also mention necessary env-variables that are necessary for nest to find the user-module related data, i.e.

`NEST_MODULE_PATH` for localization of .so / .dylib
`SLI_PATH` to find .sli scripts
When testing the changes on ubuntu with previous versions of cmake, `randomtest` and `sli` target did not find gsl symbols… adding the library explicitly to `target_link_libraries` solved this.
@tammoippen tammoippen force-pushed the tammoippen:includes branch from 28a72fb to 50d34ac Nov 14, 2017
@tclose
Copy link

@tclose tclose commented Nov 26, 2017

Is there anything holding up this PR? I am hoping to prepare a Homebrew formula for my Pype9 package but need these changes in the master.

@Silmathoron
Copy link
Member

@Silmathoron Silmathoron commented Nov 27, 2017

Yep, we're waiting for the review of @jougs ;P

@jougs
jougs approved these changes Nov 28, 2017
Copy link
Contributor

@jougs jougs left a comment

Many thanks for the fix and sorry for the long delay. Pre-christmas madness seems to kick in early this year ;-)

@jougs jougs merged commit 2fa573a into nest:master Nov 28, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@tammoippen tammoippen deleted the tammoippen:includes branch Nov 30, 2017
@tclose
Copy link

@tclose tclose commented Dec 4, 2017

Thanks all, that is great.

Do you have a rough ETA of when these changes will make it into a stable release? Will there be a 2.14.1?

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

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.