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

boost builds on default channel incompatible with newer python builds #9078

Closed
bp-kelley opened this issue Apr 3, 2018 · 7 comments
Closed

Comments

@bp-kelley
Copy link

Actual Behavior

It appears that when conda switched over to statically linked pythons, packages in the default channel were not updated accordingly. Namely boost and boost python. Building from the anaconda or condo-forge channels work correctly.

The error we are seeing is related to this one:

https://stackoverflow.com/questions/48289858/fatal-error-in-extension-pythreadstate-get-no-current-thread-when-using-swig-w

Expected Behavior

The build should work :)

Steps to Reproduce

Here is a small example extracted from the rdkit sources (https://github.com/rdkit)

git clone https://github.com/bp-kelley/rdkit-conda-issue
conda build rdkit-conda-issue

segfaults in the test.

conda build rdkit-conda-issue -c anaconda
conda build rdkit-conda-issue -c conda-forge

do not have this issue.

Anaconda or Miniconda version:

conda 4.3.34 (and earlier)

Operating System:

OSX

conda info
Current conda install:

               platform : osx-64
          conda version : 4.3.34
       conda is private : False
      conda-env version : 4.3.34
    conda-build version : 3.8.0
         python version : 3.6.0.final.0
       requests version : 2.12.4
       root environment : /Users/kellebr5/miniconda36  (writable)
    default environment : /Users/kellebr5/miniconda36
       envs directories : /Users/kellebr5/miniconda36/envs
                          /Users/kellebr5/.conda/envs
          package cache : /Users/kellebr5/miniconda36/pkgs
                          /Users/kellebr5/.conda/pkgs
           channel URLs : https://repo.continuum.io/pkgs/main/osx-64
                          https://repo.continuum.io/pkgs/main/noarch
                          https://repo.continuum.io/pkgs/free/osx-64
                          https://repo.continuum.io/pkgs/free/noarch
                          https://repo.continuum.io/pkgs/r/osx-64
                          https://repo.continuum.io/pkgs/r/noarch
                          https://repo.continuum.io/pkgs/pro/osx-64
                          https://repo.continuum.io/pkgs/pro/noarch
            config file : None
             netrc file : None
           offline mode : False
             user-agent : conda/4.3.34 requests/2.12.4 CPython/3.6.0 Darwin/17.5.0 OSX/10.13.4    
                UID:GID : 1506476385:20
@mingwandroid
Copy link

mingwandroid commented Apr 3, 2018

packages in the default channel were not updated accordingly

This is incorrect. This is a user error.

python3-config --cflags is not what you should use to build extension modules. That is for embedding Python.

@mingwandroid
Copy link

In particular, why do you think you should add -lpython3.6m when you are linking to a statically linked Python executable?

@mingwandroid
Copy link

mingwandroid commented Apr 3, 2018

Here you go. This should work fine on Linux and macOS, not sure about Windows though.

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 0a5ca82..7d2822d 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -50,6 +50,26 @@ if(RDK_BUILD_PYTHON_WRAPPERS)
         OUTPUT_STRIP_TRAILING_WHITESPACE
       )
    endif (NOT PYTHON_INSTDIR)
+
+   execute_process(
+      COMMAND
+      ${PYTHON_EXECUTABLE} -c "from distutils import sysconfig; print(sysconfig.get_config_var('Py_ENABLE_SHARED'))"
+      OUTPUT_VARIABLE Py_ENABLE_SHARED
+      OUTPUT_STRIP_TRAILING_WHITESPACE
+   )
+
+   # See https://bugs.python.org/msg277944
+   # The "command to create shared modules". Used as variable in the "Makefile (and similar) templates to build python modules"
+   # for both in-python and third party modules. Initialized to hold the value which works for third party modules to link
+   # against the _installed_ python.
+   # We strip off the first word though (which will be the compiler name).
+   execute_process(
+      COMMAND
+      ${PYTHON_EXECUTABLE} -c "from distutils import sysconfig; print(sysconfig.get_config_var('LDSHARED').split(' ', 1)[1])"
+      OUTPUT_VARIABLE PYTHON_LDSHARED
+      OUTPUT_STRIP_TRAILING_WHITESPACE
+   )
+
    message("Python Install directory ${PYTHON_INSTDIR}")
    install(DIRECTORY rdkit DESTINATION ${PYTHON_INSTDIR}
       COMPONENT python
diff --git a/Code/RDBoost/Wrap/CMakeLists.txt b/Code/RDBoost/Wrap/CMakeLists.txt
index d76b167..c2cfab8 100644
--- a/Code/RDBoost/Wrap/CMakeLists.txt
+++ b/Code/RDBoost/Wrap/CMakeLists.txt
@@ -5,8 +5,14 @@ set_target_properties(rdBase PROPERTIES
                      LIBRARY_OUTPUT_DIRECTORY
                      ${RDK_PYTHON_OUTPUT_DIRECTORY})
 
-target_link_libraries(rdBase rdkit_base
-                     ${PYTHON_LIBRARIES} ${Boost_IMPORTED_LIBRARIES} )
+if("${Py_ENABLE_SHARED}" STREQUAL "1")
+   target_link_libraries(rdBase rdkit_base
+                        ${PYTHON_LIBRARIES} ${Boost_IMPORTED_LIBRARIES} )
+else()
+   target_link_libraries(rdBase rdkit_base
+                         ${Boost_IMPORTED_LIBRARIES} )
+   set_target_properties(rdBase PROPERTIES LINK_FLAGS ${PYTHON_LDSHARED})
+endif("${Py_ENABLE_SHARED}" STREQUAL "1")
 		     
 INSTALL(TARGETS rdBase
        LIBRARY DESTINATION ${RDKit_PythonDir} COMPONENT python)
diff --git a/build.sh b/build.sh
index b7035f9..04341a3 100644
--- a/build.sh
+++ b/build.sh
@@ -21,5 +21,5 @@ cmake \
     .
 
 
-make -j$CPU_COUNT
+make -j$CPU_COUNT VERBOSE=1
 make install

IMHO the bug lies with CMake though. It cares not a jot for how the actual Python was configured and built and makes incorrect assumptions that I am working around here.

@bp-kelley
Copy link
Author

@mingwandroid It does indeed at that.

the py_enable_shared was the bit we were missing here. Thanks for that.

@mingwandroid
Copy link

Do you not also need my changes around PYTHON_LDSHARED? It drags in:

-bundle -undefined dynamic_lookup

.. and without this, the test-case failed due to the lack of -undefined dynamic_lookup

@bp-kelley
Copy link
Author

Both were needed, I meant using sysconfig.

btw - there is also an issue with cmake trying to using both -bundle and -dynamiclib... One of them needs to be removed. While I agree this was a "user error" it is an incredibly non-obvious one since everything compiles and the tests segfault.

@mingwandroid
Copy link

CMake's support for Python takes a lowest common denominator approach of assuming 'system' python and whatever they think that entails. It is usually subtly wrong but harmlessly so on Linux - linking to libpython on distros that run static python interpreters, but the same problem is fatal for a static macOS python interpreter. I mean to spend time to see if I can make this harmless on macOS but I'm not sure it's technically possible. The macOS dynamic linker is very complex so maybe there's a way.

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

No branches or pull requests

2 participants