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

Improve handling of output vars from FindXXX.cmake #857

Merged
merged 3 commits into from Mar 6, 2018

Conversation

Projects
None yet
5 participants
@tammoippen
Contributor

tammoippen commented Nov 14, 2017

I encountered a different and consistent issue with handling XXX_LIBRARIES and XXX_INCLUDE_DIRS in the FindXXX.cmake files: they are set to XXX_LIBRARIES-NOTFOUND via find_package_handle_standard_args, which might result in the error:

CMake Error: The following variables are used in this project, but they are set to NOTFOUND.
Please set them or make sure they are set and tested correctly in the CMake files:
LTDL_LIBRARIES (ADVANCED)
    linked by target "nestkernel" in directory /Users/nest/nest-simulator/nestkernel

-- Configuring incomplete, errors occurred!

=> You cannot compile nest.

This PR applies a more consistent handling:

  • Always check for singular version of XXX_LIBRARY, XXX_INCLUDE_DIR
  • before calling find_package_handle_standard_args assign the XXX_LIBRARY to their plural form XXX_LIBRARIES - these will be the variables external users (i.e. nest) interact with.

This way XXX_LIBRARY will have the NOTFOUND value and the user code can use XXX_LIBRARIES without issues (as is recommended here: https://cmake.org/cmake/help/git-master/manual/cmake-developer.7.html#find-modules).

Further add OS X specific information about libltdl in the ConfigureSummary. (Thanks to @tomvierjahn .)

@jougs

jougs approved these changes Feb 6, 2018

This looks good to me. Many thanks for the contribution and sorry for the long time it took to review.

@jougs

This comment has been minimized.

Show comment
Hide comment
@jougs

jougs Feb 26, 2018

Contributor

@heplesser, @hakonsbm, @stinebuu: Could one of you please have a look at this? @gtrensch is currently ill and I'd like to have this merged. Thanks!

Contributor

jougs commented Feb 26, 2018

@heplesser, @hakonsbm, @stinebuu: Could one of you please have a look at this? @gtrensch is currently ill and I'd like to have this merged. Thanks!

@tomvierjahn

This comment has been minimized.

Show comment
Hide comment
@tomvierjahn

tomvierjahn Feb 27, 2018

@tammoippen, the "set to NOTFOUND" warning is annoying, indeed. However, setting XYZ_LIBRARIES and XYZ_INCLUDE_DIRS before calling find_package_handle_standard_args seems unusual to me. find_package_handle_standard_args checks if all the requirements are met. According to the CMake documentation, you set the said variables after the call, if the package has been found correctly.

For cmake/FindLTDL.cmake this'll lead to

...
find_path( LTDL_INCLUDE_DIR
    NAMES ltdl.h
    HINTS ${LTDL_ROOT_DIR}/include
    )
find_library( LTDL_LIBRARY
    NAMES ltdl
    HINTS ${LTDL_ROOT_DIR}/lib
    )
...
include( FindPackageHandleStandardArgs )
find_package_handle_standard_args( LTDL
  FOUND_VAR
    LTDL_FOUND
  REQUIRED_VARS
    LTDL_LIBRARY
    LTDL_INCLUDE_DIR
  VERSION_VAR
    LTDL_VERSION
    )

if (LTDL_FOUND)
  set( LTDL_INCLUDE_DIRS "${LTDL_INCLUDE_DIR}" )
  set( LTDL_LIBRARIES "${LTDL_LIBRARY}" )
endif()

tomvierjahn commented Feb 27, 2018

@tammoippen, the "set to NOTFOUND" warning is annoying, indeed. However, setting XYZ_LIBRARIES and XYZ_INCLUDE_DIRS before calling find_package_handle_standard_args seems unusual to me. find_package_handle_standard_args checks if all the requirements are met. According to the CMake documentation, you set the said variables after the call, if the package has been found correctly.

For cmake/FindLTDL.cmake this'll lead to

...
find_path( LTDL_INCLUDE_DIR
    NAMES ltdl.h
    HINTS ${LTDL_ROOT_DIR}/include
    )
find_library( LTDL_LIBRARY
    NAMES ltdl
    HINTS ${LTDL_ROOT_DIR}/lib
    )
...
include( FindPackageHandleStandardArgs )
find_package_handle_standard_args( LTDL
  FOUND_VAR
    LTDL_FOUND
  REQUIRED_VARS
    LTDL_LIBRARY
    LTDL_INCLUDE_DIR
  VERSION_VAR
    LTDL_VERSION
    )

if (LTDL_FOUND)
  set( LTDL_INCLUDE_DIRS "${LTDL_INCLUDE_DIR}" )
  set( LTDL_LIBRARIES "${LTDL_LIBRARY}" )
endif()
@tomvierjahn

This comment has been minimized.

Show comment
Hide comment
@tomvierjahn

tomvierjahn Feb 27, 2018

Apart from my previous comment, the output if libtool is missing on Mac looks good to me. Thanks!

tomvierjahn commented Feb 27, 2018

Apart from my previous comment, the output if libtool is missing on Mac looks good to me. Thanks!

@tammoippen

This comment has been minimized.

Show comment
Hide comment
@tammoippen

tammoippen Feb 28, 2018

Contributor

@tomvierjahn This sounds valid! And shows, how documentation and implementation can differ / diverge. When I wrote the changes, I oriented on FindGSL.cmake from cmake: they set those variables before the find_package_handle_standard_args, see here https://github.com/Kitware/CMake/blob/master/Modules/FindGSL.cmake#L118 . I would suggest to change this in all our files, except the FindGSL.cmake to keep similarity to the original.

Contributor

tammoippen commented Feb 28, 2018

@tomvierjahn This sounds valid! And shows, how documentation and implementation can differ / diverge. When I wrote the changes, I oriented on FindGSL.cmake from cmake: they set those variables before the find_package_handle_standard_args, see here https://github.com/Kitware/CMake/blob/master/Modules/FindGSL.cmake#L118 . I would suggest to change this in all our files, except the FindGSL.cmake to keep similarity to the original.

@tammoippen

This comment has been minimized.

Show comment
Hide comment
@tammoippen

tammoippen Feb 28, 2018

Contributor

@tomvierjahn I have applied the suggested changes. Please have a look.

Contributor

tammoippen commented Feb 28, 2018

@tomvierjahn I have applied the suggested changes. Please have a look.

@terhorstd terhorstd added this to the NEST 2.16 milestone Mar 5, 2018

tammoippen added some commits Nov 14, 2017

Improve handling of output vars from FindXXX.cmake
I encountered a different and consistent issue with handling `XXX_LIBRARIES` and `XXX_INCLUDE_DIRS` in the `FindXXX.cmake` files: they are set to `XXX_LIBRARIES-NOTFOUND` via `find_package_handle_standard_args`, which results in the error:

```
CMake Error: The following variables are used in this project, but they are set to NOTFOUND.
Please set them or make sure they are set and tested correctly in the CMake files:
LTDL_LIBRARIES (ADVANCED)
    linked by target "nestkernel" in directory /Users/nest/nest-simulator/nestkernel

-- Configuring incomplete, errors occurred!
```

=> You cannot compile nest.

Here a more consistent handling is applied:
Always check for singular version of `XXX_LIBRARY`, before calling `find_package_handle_standard_args` assign the `XXX_LIBRARY` to their plural form `XXX_LIBRARIES` - these will be the variables external users interact with. This way `XXX_LIBRARY` will have the `NOTFOUND` value and the user code can use `XXX_LIBRARIES` without issues (as is recomended: https://cmake.org/cmake/help/git-master/manual/cmake-developer.7.html#find-modules
@tomvierjahn

Looks good to me. Output for missing libtool on Mac OS works correctly. Homebrew's libtool is recognised correctly, disabling the warning.

@jougs

This comment has been minimized.

Show comment
Hide comment
@jougs

jougs Mar 6, 2018

Contributor

Many thanks to all involved people. I think we can waive the review by @gtrensch given the review by @tomvierjahn. @abigailm, can you please merge if you agree with my assessment?

Contributor

jougs commented Mar 6, 2018

Many thanks to all involved people. I think we can waive the review by @gtrensch given the review by @tomvierjahn. @abigailm, can you please merge if you agree with my assessment?

@abigailm abigailm merged commit b2807a7 into nest:master Mar 6, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment