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

mimic: build/ops: Cython 0.29 removed support for subinterpreters: raises ImportError: Interpreter change detected ... #27971

Merged
merged 3 commits into from Jul 15, 2019

Conversation

@smithfarm smithfarm self-assigned this May 6, 2019

@smithfarm smithfarm added this to the mimic milestone May 6, 2019

@smithfarm smithfarm added bug fix core build/ops and removed core labels May 6, 2019

@smithfarm smithfarm requested review from tchaikov, tserong and rjfd May 6, 2019

@smithfarm smithfarm changed the title mimic: Cython 0.29 removed support for subinterpreters: raises ImportError: Interpreter change detected ... mimic: build/ops: Cython 0.29 removed support for subinterpreters: raises ImportError: Interpreter change detected ... May 6, 2019

@tserong

tserong approved these changes May 6, 2019

@yuriw

This comment has been minimized.

Copy link
Contributor

commented May 6, 2019

@yuriw

This comment has been minimized.

Copy link
Contributor

commented May 6, 2019

@@ -63,7 +67,9 @@ function(distutils_install_cython_module name)
install(CODE "
set(ENV{CC} \"${PY_CC}\")
set(ENV{LDSHARED} \"${PY_LDSHARED}\")
set(ENV{CPPFLAGS} \"-iquote${CMAKE_SOURCE_DIR}/src/include\")
set(ENV{CPPFLAGS} \"-iquote${CMAKE_SOURCE_DIR}/src/include
-D'void0=dead_function\(void\)' \

This comment has been minimized.

Copy link
@tserong

tserong May 7, 2019

Contributor

Um. Guessing. Could the backslash at the end of this line be causing problems with string parsing? Or, conversely, the lack of backslash on the previous line? That said, isn't it odd that it didn't fail when #27067 was tested?

This comment has been minimized.

Copy link
@tchaikov

tchaikov Jul 11, 2019

Contributor

@tserong the reason why #27067 didn't fail the test is that we are using cmake3 since #22896.

@rgpublic

This comment has been minimized.

Copy link

commented Jun 17, 2019

Any possibility to speed this up? AFAIU there is only a pending reviewer (@rjfd), right? Currently we have a completely broken Ceph on Ubuntu 19.04 where no modules are loaded :-(
See also, for example: https://ruffell.nz/programming/writeups/2019/05/17/deploying-a-ceph-cluster-in-ubuntu-19.04.html

@smithfarm

This comment has been minimized.

Copy link
Contributor Author

commented Jun 26, 2019

test this please

@edsokolowsk

This comment has been minimized.

Copy link

commented Jun 26, 2019

We are impacted by this issue.

Q1. What is the status of this branch release since we require ceph mimic running on Ubuntu 19.04?

Q2. Is there any workaround , say using a previous release of Cython that supports subinterpreters?

@smithfarm

This comment has been minimized.

Copy link
Contributor Author

commented Jul 11, 2019

retest this please

tserong added some commits Dec 17, 2018

cmake: remove cython 0.29's subinterpreter check
cython 0.29 introduced a check which prevents multiple python
subinterpreters from loading the same module:

cython/cython@7e27c7c

Unfortunately, this completely breaks ceph-mgr.  Until we can
figure out a better long term solution, this commit removes
cython's subinterpreter check, via some careful abuse of the
C preprocessor.

This works because when cython is invoked, it first generates
some C code, then compiles it.  We know it's going to generate
C code including:

  int __Pyx_check_single_interpreter(void) { ... }

and:

  if (__Pyx_check_single_interpreter())
      return NULL;

So, we can do the following:

  #define void0 dead_function(void)
  #define __Pyx_check_single_interpreter(ARG)=ARG ## 0

This replaces the call to __Pyx_check_single_interpreter()
with a literal 0, removing the subinterpreter check.

The void0 dead_function(void) thing is necessary because
the __Pyx_check_single_interpreter() macro also clobbers
that function definition, so we need to make sure it's
replaced with something that works as a function definition.

Fixes: https://tracker.ceph.com/issues/37472
Signed-off-by: Tim Serong <tserong@suse.com>
(cherry picked from commit 3bde34a)

Conflicts:
	cmake/modules/Distutils.cmake
- mimic doesn't have the immediately preceding ccache stuff
cmake: remove cython 0.29's subinterpreter check during install
Commit 3bde34a removed cython 0.29's subinterpreter check when
building the various python modules during `make`, but unforunately
they're *rebuilt* during `make install`, with the rebuild overwriting
the original build.  The original fix was of course missing from the
install stage...

Fixes: https://tracker.ceph.com/issues/38788
Signed-off-by: Tim Serong <tserong@suse.com>
(cherry picked from commit 108462e)
@rjfd

rjfd approved these changes Jul 11, 2019

Copy link
Contributor

left a comment

lgtm

@smithfarm smithfarm force-pushed the smithfarm:wip-39593-mimic branch from 00aecf7 to 8f8784f Jul 11, 2019

@smithfarm

This comment has been minimized.

@smithfarm

This comment has been minimized.

Copy link
Contributor Author

commented Jul 11, 2019

CMake Error at cmake/modules/Distutils.cmake:67 (install):
  Syntax error in cmake code at

    /home/jenkins-build/build/workspace/ceph-dev-new-build/ARCH/x86_64/AVAILABLE_ARCH/x86_64/AVAILABLE_DIST/centos7/DIST/centos7/MACHINE_SIZE/huge/release/13.2.6-91-g8f8784f/rpm/el7/BUILD/ceph-13.2.6-91-g8f8784f/cmake/modules/Distutils.cmake:67

  when parsing string

    
      set(ENV{CC} \"${PY_CC}\")
      set(ENV{LDSHARED} \"${PY_LDSHARED}\")
      set(ENV{CPPFLAGS} \"-iquote${CMAKE_SOURCE_DIR}/src/include
                          -D'void0=dead_function\(void\)' \
                          -D'__Pyx_check_single_interpreter\(ARG\)=ARG \#\# 0'\")
      set(ENV{LDFLAGS} \"-L${CMAKE_LIBRARY_OUTPUT_DIRECTORY}\")
      set(ENV{CYTHON_BUILD_DIR} \"${CMAKE_CURRENT_BINARY_DIR}\")
      set(ENV{CEPH_LIBDIR} \"${CMAKE_LIBRARY_OUTPUT_DIRECTORY}\")

  

      set(options --prefix=${CMAKE_INSTALL_PREFIX})
      if(DEFINED ENV{DESTDIR})
        if(EXISTS /etc/debian_version)
          list(APPEND options --install-layout=deb)
        endif()
        list(APPEND options --root=\$ENV{DESTDIR})
      else()
        list(APPEND options --root=/)
      endif()
      execute_process(
         COMMAND
             ${PYTHON${PYTHON_VERSION}_EXECUTABLE} ${CMAKE_CURRENT_SOURCE_DIR}/setup.py
             build --verbose --build-base ${CYTHON_MODULE_DIR}
             --build-platlib ${CYTHON_MODULE_DIR}/lib.${PYTHON${PYTHON_VERSION}_VERSION_MAJOR}
             build_ext --cython-c-in-temp --build-temp ${CMAKE_CURRENT_BINARY_DIR} --cython-include-dirs ${PROJECT_SOURCE_DIR}/src/pybind/rados
             install \${options} --single-version-externally-managed --record /dev/null
             egg_info --egg-base ${CMAKE_CURRENT_BINARY_DIR}
             --verbose
         WORKING_DIRECTORY \"${CMAKE_CURRENT_SOURCE_DIR}\"
         RESULT_VARIABLE install_res)
      if(NOT \"\${install_res}\" STREQUAL 0)
        message(FATAL_ERROR \"Failed to build and install ${name} python module\")
      endif()
    

  syntax error, unexpected cal_SYMBOL, expecting $end (252)
Call Stack (most recent call first):
  src/pybind/rados/CMakeLists.txt:3 (distutils_install_cython_module)


-- Configuring incomplete, errors occurred!
@smithfarm

This comment has been minimized.

Copy link
Contributor Author

commented Jul 11, 2019

So, yeah, whichever version of CentOS that mimic is building on in Shaman is installing:

cmake-2.8.12.2-2.el7.x86_64

i.e. cmake 2.8.12, which does not understand the backslash syntax for line continuation.

That said, isn't it odd that it didn't fail when #27067 was tested?

Not really, if master is building on a newer version of CentOS with a newer version of cmake.

@tchaikov

This comment has been minimized.

Copy link
Contributor

commented Jul 11, 2019

as an alternative, we could backport #22896 .

deb,rpm,do_cmake: switch to cmake3
Signed-off-by: Kefu Chai <kchai@redhat.com>
(cherry picked from commit af2c91a)

@smithfarm smithfarm force-pushed the smithfarm:wip-39593-mimic branch from 10a4ed5 to ce19141 Jul 11, 2019

@smithfarm

This comment has been minimized.

Copy link
Contributor Author

commented Jul 11, 2019

@yuriw

This comment has been minimized.

Copy link
Contributor

commented Jul 11, 2019

@yuriw yuriw merged commit 8b7de8e into ceph:mimic Jul 15, 2019

4 checks passed

Docs: build check OK - docs built
Details
Signed-off-by all commits in this PR are signed
Details
Unmodified Submodules submodules for project are unmodified
Details
make check make check succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.