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

targetLinkLibrariesWithDynamicLookup: Backport module from scikit-build #80

Merged
merged 4 commits into from Dec 20, 2016

Conversation

Projects
None yet
3 participants
@blowekamp
Member

blowekamp commented Dec 6, 2016

Note that the name of the function remains prefixed with "sitk_"

Change-Id: I9e3eba4f3de436b297a2547c4f2a1346e6ecfec3

@blowekamp

This comment has been minimized.

Member

blowekamp commented Dec 6, 2016

@jcfr
Some systems and some language's dev packages don't contain the language library, and require the usage of dynamic look up in the executable for the language symbols. The manylinux docker image is one case of this. Previously the CMake variable "SITK_UNDEFINED_SYMBOLS_ALLOWED" was used to determine if the libraries were required.

https://github.com/SimpleITK/SimpleITK/blob/master/CMake/sitkLanguageOptions.cmake#L23-L27

I don't see a replacement for this variable. Any suggestions on how to update the behavior?

@blowekamp

This comment has been minimized.

Member

blowekamp commented Dec 16, 2016

@jcfr Any feedback or suggestions on this issue?

@jcfr

This comment has been minimized.

Contributor

jcfr commented Dec 16, 2016

When using manylinux, the following is done:

https://github.com/SimpleITK/SimpleITKPythonPackage/blob/426b971c730ac5ceaf81f87fac0142192b9031f1/scripts/internal/manylinux-build-wheels.sh#L26-L40

This allows to keep find_package(PythonLibs) happy while ensuring no target is linking against the python library. Simply using QUIET would not play the same role.

@blowekamp

This comment has been minimized.

Member

blowekamp commented Dec 16, 2016

What is missing from the solution of marking a find language library as QUIET, and then not linking against it?

The prior solution does not include a check to ensure linking does not occur, but it does not require custom configuration when a system or language is encountered without the libraries. The problem ( or purposeful intent ) of language development packages not including libraries has occurred several time in the real world ( outside of building on manylinux). It is necessary to continue supporting these systems output the box.

An improved solution to the existing one is welcomed, if it meets the requirements.

@jcfr

This comment has been minimized.

Contributor

jcfr commented Dec 16, 2016

solution of marking a find language library as QUIET, and then not linking against it?

Looking at your previous link, I missed the part where it doesn't link against it. It looks like this is done in function sitk_target_link_libraries_with_dynamic_lookup implemented in sitkTargetLinkLibrariesWithDynamicLookup.cmake#L81-L89

It turns out that the implementation of that same function in scikit-build also conditionally links against the python library. We are good on that front.

The problem is in two locations:

and

Problem (1)

For the problem (1), changing that code from

if ( SITK_UNDEFINED_SYMBOLS_ALLOWED )
  set( _QUIET_LIBRARY "QUIET" )
else()
  set( _QUIET_LIBRARY "REQUIRED" )
endif()

to

check_dynamic_lookup(MODULE
  SHARED
  SITK_UNDEFINED_SYMBOLS_ALLOWED
  unused
  )
if ( SITK_UNDEFINED_SYMBOLS_ALLOWED )
  set( _QUIET_LIBRARY "QUIET" )
else()
  set( _QUIET_LIBRARY "REQUIRED" )
endif()

should address the issue.

Similarly to the other CMake modules, I will make sure it is documented on read the docs.

In the mean time, associated documentation is here: https://github.com/scikit-build/scikit-build/blob/d592c8084a7e0502d9da9e7aacf0e465be40cc67/skbuild/resources/cmake/targetLinkLibrariesWithDynamicLookup.cmake#L61-L92

Problem (2)

For the problem (2) in SimpleITKPythonPackage/CMakeLists.txt, the code:


  if(NOT DEFINED PYTHON_INCLUDE_DIR
     OR NOT DEFINED PYTHON_LIBRARY
     OR NOT DEFINED PYTHON_EXECUTABLE)

    find_package ( PythonLibs REQUIRED )
    find_package ( PythonInterp REQUIRED )

  endif()

should be changed into

  # TODO: Download  https://raw.githubusercontent.com/scikit-build/scikit-build/v0.4.0/skbuild/resources/cmake/targetLinkLibrariesWithDynamicLookup.cmake
  include(targetLinkLibrariesWithDynamicLookup)
  check_dynamic_lookup(MODULE
    SHARED
    SITK_UNDEFINED_SYMBOLS_ALLOWED
    unused
    )

  if(NOT DEFINED PYTHON_INCLUDE_DIR
       OR NOT DEFINED PYTHON_LIBRARY
       OR NOT DEFINED PYTHON_EXECUTABLE)
    
    if ( SITK_UNDEFINED_SYMBOLS_ALLOWED )
      set( _QUIET_LIBRARY "QUIET" )
    else()
      set( _QUIET_LIBRARY "REQUIRED" )
    endif()
    find_package ( PythonLibs ${_QUIET_LIBRARY})
    find_package ( PythonInterp REQUIRED )

  endif()

Todo:

  • Generate documentation for targetLinkLibrariesWithDynamicLookup.cmake
  • Improve check_dynamic_lookup so that the fourth argument link_flags_var is optional
  • Even better, support signature check_dynamic_lookup(SITK_UNDEFINED_SYMBOLS_ALLOWED) that would be equivalent of calling check_dynamic_lookup(MODULE SHARED SITK_UNDEFINED_SYMBOLS_ALLOWED unused)
@blowekamp

This comment has been minimized.

Member

blowekamp commented Dec 16, 2016

Thanks!

And it's more that just Python. All language library linking goes through sitk_target_link_libraries_with_dynamic_lookup.

jcfr added a commit to scikit-build/scikit-build that referenced this pull request Dec 19, 2016

targetLinkLibrariesWithDynamicLookup: extend check_dynamic_lookup API
* update long signature: "LinkFlagsVar" is now optional

* add support for short signature: check_dynamic_lookup(<ResultVar>)

See SimpleITK/SimpleITK#80
@jcfr

This comment has been minimized.

Contributor

jcfr commented Dec 19, 2016

scikit-build/scikit-build#230 is adding documentation along with support for the short signature check_dynamic_lookup(<ResultVar>)

jcfr added a commit to scikit-build/scikit-build that referenced this pull request Dec 19, 2016

targetLinkLibrariesWithDynamicLookup: extend check_dynamic_lookup API
* update long signature: "LinkFlagsVar" is now optional

* add support for short signature: check_dynamic_lookup(<ResultVar>)

See SimpleITK/SimpleITK#80

jcfr and others added some commits Sep 25, 2016

targetLinkLibrariesWithDynamicLookup: Backport module from scikit-build
Replacing The Original SimpleITK TargetLinkLibrariesWithDynamicLookup
with the now upstream scikit-build CMake module. The goal is to have
this module mature in the scikit-build repository before CMake
adopting it.

Change-Id: I9e3eba4f3de436b297a2547c4f2a1346e6ecfec3

@blowekamp blowekamp force-pushed the blowekamp:BackportDynamicLookupCMakeModule branch from 4b22c9b to 9086c4b Dec 19, 2016

blowekamp added some commits Dec 19, 2016

Use CMake method to set SITK_UNDEFINED_SYMBOLS_ALLOWED
The scikit-build upstream CMake module no longer defined extra CMake
variables. The variable need to be explicitly passed to get
initialized.
Address CMP0054 warning with "unused" string/variable
Use standard regex MATCH approach to avoid a string being confused
with a variable. This patch addressed the following CMake warning:

CMake Warning (dev) at
CMake/sitkTargetLinkLibrariesWithDynamicLookup.cmake:466 (if):
Policy CMP0054 is not set: Only interpret if() arguments as
variables or
keywords when unquoted. Run "cmake --help-policy CMP0054" for
policy
details. Use the cmake_policy command to set the policy and
suppress this
warning.

Quoted variables like "unused" will no longer be dereferenced when
the
policy is set to NEW. Since the policy is not set the OLD behavior
will be
used.

@blowekamp blowekamp force-pushed the blowekamp:BackportDynamicLookupCMakeModule branch from 0917ff5 to 0027799 Dec 19, 2016

@blowekamp

This comment has been minimized.

Member

blowekamp commented Dec 19, 2016

CircleCI continuous build's configuration looks OK.

Merging into SimpleITK for nightly testing across platforms.

@kwrobot kwrobot merged commit 0027799 into SimpleITK:master Dec 20, 2016

1 check passed

ci/circleci Your tests passed on CircleCI!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment