cmake: enabled py3 only build #20064

tchaikov · 2018-01-23T05:46:29Z

No description provided.

smithfarm · 2018-01-23T08:24:33Z

@tchaikov Thanks! I'll cherry-pick these commits into #20018 and work with them further. I have a py3-only environment in the OBS that I'm testing in.

smithfarm · 2018-01-23T15:06:14Z

@tchaikov Nope. BuildBoost still can't find the python headers:

[  100s] warning: No python installation configured and autoconfiguration
[  100s] note: failed.  See http://www.boost.org/libs/python/doc/building.html
[  100s] note: for configuration instructions or pass --without-python to
[  100s] note: suppress this message and silently skip all Boost.Python targets

The build is currently running, but I expect it to fail on #include <Python.h>. This header file is there because python3-devel RPM is installed as a build dependency. The file name is /usr/include/python3.6m/Python.h

I will look how boost is packaged in openSUSE - maybe I can find a clue.

tchaikov · 2018-01-23T15:08:36Z

@smithfarm no worries. i will try again in my testbed tomorrow.

smithfarm · 2018-01-23T15:15:26Z

Based on my reading of https://build.opensuse.org/package/view_file/openSUSE:Factory:Staging:O/boost/boost.spec?expand=1 the py3-only build needs to create a jam file with something like the following contents:

cat << EOF >> user-config-py3.jam
using python
	: %{python3_version}
	: %{_bindir}/python3
	: %{_includedir}/python%{python3_version}%{py3_abiflags}
	:
	:
	: .%{py3_soflags}
	: %{py3_abiflags}
	;
%if %{with build_mpi}
using mpi ;
%endif
EOF

I guess the RPM/DEB packaging will need to explicitly specify the Python 3 version on the cmake command line.

smithfarm · 2018-01-23T15:42:42Z

cmake/modules/BuildBoost.cmake

+      " : ${PYTHON_VERSION_STRING}"
+      " : ${PYTHON_EXECUTABLE}"
+      " : ${PYTHON_INCLUDE_DIRS}"
+      " : ${PYTHON_LIBRARIES}"


It seems that:

in the py2/py3 environment, these variables are populated by FindPython{Interp,Libs}.cmake and they point to Python 2.7

in the py3-only environment, FindPython{Interp,Libs}.cmake is not run. Instead, FindPython3{Interp,Libs}.cmake is run and does not populate these variables. Instead of PYTHON_VERSION_STRING, for example, it populates PYTHON3_VERSION_STRING.

@tchaikov Please ignore the above comment - I found that FindPython{Interp,Libs}.cmake are properly populated in the py3-only env.

smithfarm · 2018-01-23T16:02:27Z

cmake/modules/BuildBoost.cmake

+  if(with_python GREATER -1)
+    file(APPEND ${source_dir}/user-config.jam
+      "using python"
+      " : ${PYTHON_VERSION_STRING}"


From http://www.boost.org/doc/libs/1_58_0/libs/python/doc/building.html#python-configuration-parameters

version
the version of Python to use. Should be in Major.Minor format, for example, 2.3. Do not include the subminor version (i.e. not 2.5.1).

Except ${PYTHON_VERSION_STRING} expands to a value that DOES include the subminor version:

PYTHON_VERSION_STRING 3.6.4

Now testing:

- " : ${PYTHON_VERSION_STRING}" + " : ${PYTHON_VERSION_MAJOR}.${PYTHON_VERSION_MINOR}"

smithfarm · 2018-01-23T16:25:18Z

I've tried a number of things, but nothing seems to make the "No python installation configured and autoconfiguration" warning go away. What's worse, I can't find the source code where this warning is generated, so I can't see what's triggering it.

smithfarm · 2018-01-23T16:46:01Z

jenkins re-test this please

smithfarm · 2018-01-23T17:38:46Z

[  5%] Performing build step for 'Boost'
error: Configuration file 'user-config.jam' not found in '/home/jenkins-build/build/workspace/ceph-pull-requests/build/boost/src/Boost'.

CMakeFiles/Boost.dir/build.make:113: recipe for target 'boost/src/Boost-stamp/Boost-build' failed
make[3]: *** [boost/src/Boost-stamp/Boost-build] Error 1

smithfarm · 2018-01-23T17:42:10Z

error: Configuration file 'user-config.jam' not found in '/home/jenkins-build/build/workspace/ceph-pull-requests/build/boost/src/Boost'

^^^ this is weird, because BuildBoost.cmake sets source_dir to ...something.../src/boost

[EDITED] . . . but only when the src/boost directory is already populated (e.g. when using make-dist). When BuildBoost.cmake populates that directory itself by downloading the tarball, source_dir gets set to .../src/Boost :-(

smithfarm · 2018-01-23T18:00:46Z

From the Jenkins console log:

-- extracting...
     src='/home/jenkins-build/build/workspace/ceph-pull-requests/build/boost/src/boost_1_66_0.tar.bz2'
     dst='/home/jenkins-build/build/workspace/ceph-pull-requests/build/boost/src/Boost'
-- extracting... [tar xfz]

(strange that it says "tar xfz" when the tarball is compressed with bz2 - shouldn't it say "tar xfj"?)

smithfarm · 2018-01-23T19:16:59Z

I will schedule a rados run on this, along with #19122 and #20077

tchaikov · 2018-01-24T02:02:16Z

changelog

adjust the order in which user-config.jam is written so it's closer to where user_config is set. it's just a cosmetic change.
set buildid to an empty string if current component is not python.

smithfarm

Thanks, @tchaikov !

smithfarm · 2018-01-25T05:58:39Z

This PR passed a rados run here: http://pulpito.ceph.com/smithfarm-2018-01-24_19:46:55-rados-wip-smithfarm-testing-distro-basic-smithi/

198 of 202 tests passed.

Failed tests:

http://tracker.ceph.com/issues/22656 (known bug in master), did not reproduce on rerun
"2018-01-25 01:54:14.602444 mgr.z client.4375 172.21.15.193:0/3577525578 1 : cluster [ERR] Unhandled exception from module 'prometheus' while running on mgr.z: error(98, 'Address already in use')" in cluster log
"2018-01-25 01:40:33.702645 mon.f mon.0 172.21.15.94:6789/0 118 : cluster [WRN] Health check failed: 1 slow ops, oldest one blocked for 33 sec (SLOW_OPS)" in cluster log, did not reproduce on rerun.

One "dead" test:

"rados/singleton/{all/thrash-eio.yaml msgr-failures/many.yaml msgr/async.yaml objectstore/filestore-xfs.yaml rados.yaml}" - complains about lost PGs, never seems to finish

smithfarm · 2018-01-25T09:32:32Z

I'm re-running the failed tests. The "Address already in use" from module "prometheus" failure looks like it might be relevant to this PR. Reading the log - http://qa-proxy.ceph.com/teuthology/smithfarm-2018-01-24_19:46:55-rados-wip-smithfarm-testing-distro-basic-smithi/2106392/teuthology.log - I can see that the module selftests (tasks.mgr.test_module_selftest) complete successfully. Several awful-sounding log messages (e.g. MGR_DOWN, etc.) are whitelisted, so maybe the "Address already in use" message could be whitelisted as well?

UPDATE: in two subsequent reruns, the same test fails on a different log message: "2018-01-25 09:32:00.117463 mgr.y client.4393 172.21.15.28:0/3800875254 1 : cluster [ERR] Unhandled exception from module 'prometheus' while running on mgr.y: IOError("Port 9283 not free on '::'",)" in cluster log

UPDATE 2: the same test fails on master as well: http://pulpito.ceph.com/smithfarm-2018-01-25_11:32:09-rados-master-distro-basic-smithi/ (but in this case it fails outright, not on a log message)

UPDATE 3: the test should be fixed by #20047

smithfarm · 2018-01-25T12:08:31Z

@tchaikov I did a rados run with positive results, but now this needs a rebase.

Signed-off-by: Kefu Chai <kchai@redhat.com>

tchaikov · 2018-01-25T14:32:57Z

@smithfarm rebased on master, and repushed.

jcsp · 2018-01-25T17:32:47Z

src/mgr/PyModule.cc


    // Initialize module
+#if PY_MAJOR_VERSION >= 3
+    PyObject *ceph_module = PyModule_Create(&ceph_module_def);


We switch to PyModule_Create here but continue to use Py_InitModule for ceph_logger above. Also it seems like the ceph_logger_module struct is unused, so I guess the intent was to migrate both calls?

thanks @jcsp , the change related to ceph_logger_module was lost while rebasing against master. and i put it back in the latest change.

jcsp · 2018-01-25T17:42:29Z

I've checked this is compiling and working in my local (python2) environment, so combined with the earlier rados run I think we can be reasonably confident it isn't breaking anything

jcsp

I'm not an expert in the cmake bits but it all seems reasonable, so I'm happy with this once the Py_InitModule/PyModule_Create comment is done.

smithfarm · 2018-01-25T17:48:36Z

CMakeLists.txt

-  set(Python_ADDITIONAL_VERSIONS 2.7)
-  find_package(PythonInterp 2.7 REQUIRED)
-  find_package(PythonLibs 2.7 REQUIRED)
+  set(MGR_PYTHON_VERSION "2.7" CACHE STRING "python runtime for running mgr plugins")


could we put a comment above this line:

# MGR_PYTHON_VERSION specifies the required Python major version and minimum minor version # within that major version. # To build in a Python 3-only environment, override with, e.g. -DMGR_PYTHON_VERSION=3.6

@smithfarm this setting set the minimal required version of python, not the major and minor version. say, if we need to use python3 for running mgr plugins, but do not care which minor version is:

-DMGR_PYTHON_VERSION=3

would suffice. i updated the wording of the doc string of this cached string. HTH.

tchaikov · 2018-01-26T05:06:26Z

changelog

revised the docstring of MGR_PYTHON_VERSION
added the py3 change related to ceph_logger_module back
made module_methods and ceph_module_def static variables, as the python module variable ceph_module initialized using them outlives the function scope. and the module contains a reference to the ceph_module_def under python3.

smithfarm · 2018-01-26T05:50:53Z

CMakeLists.txt

-  find_package(PythonInterp 2.7 REQUIRED)
-  find_package(PythonLibs 2.7 REQUIRED)
+  set(MGR_PYTHON_VERSION "2.7" CACHE
+    STRING "minimal required version of python runtime for running mgr plugins")


@tchaikov Cmake is a build system, so if a special non-default setting is necessary to build in a py3-only environment, the parameter definition/explanation should note that.

I will try the py3-only build again without overriding this value, and if it fails (as I remember it doing), will post the exact error message.

Thanks for suggesting -DMGR_PYTHON_VERSION=3 - I'll try that as well.

In short, I find this help text "minimal required version of python runtime for running mgr plugins" to be incomplete in the sense that it omits a fact that is important for someone trying to build (not just run) MGR in a py3-only environment.

@smithfarm as you noted, cmake is a building system. so

a fact that is important for someone trying to build (not just run) MGR in a py3-only environment.

i think the help message implies that one need to ready the building environment also. if we went further in this direction, shall we note that LTTng is both a build-time and run-time dependency/feature as well? the list can grow longer.

@tchaikov So, the point I'm trying to make is that the current help string says (or implies) that 2.7 is the minimal required version to build. But this only hold true on a system with Python 2 installed. In a Python 3 (only) system, the minimum required version is 3.

That's why I suggested a comment to help folks who might be affected by the build failure because they neglect to override this default (cached) value.

please note a a cached variable is a user-facing setting. see https://cmake.org/cmake/help/v3.6/command/set.html#set-cache-entry .

OK, so you're saying it should be obvious to the user.

When the parameter definition says this specifies the minimum required version, the default is 2.7, and I have 3, then logic dictates that I don't need to change the setting. But in reality I do need to change it. This is not obvious.

@smithfarm yes, it should be obvious to a maintainer who's working on cmake projects.

Anyway, it's not important - I can add the comment in a separate PR.

* add WITH_PYTHON2 option, so we can build python3 bindings only. * change the default value of WITH_PYTHON3 option to "OFF", as the option() command in cmake only allow the initial value to be "ON" or "OFF". we could use a cached string for this option, but i think it would be more explicit to continue using the option() command. * fix the installation dir of "ceph_rest_api.py". please note, we still have a *default* python version, which is specified by the last element of ${py_vers}. for instance, ${PYTHON_VERSION} will be 3 if ${py_vers} is 2;3. in this change, 2 is still the default python version, if both WITH_PYTHON2 and WITH_PYTHON3 are enabled. Signed-off-by: Kefu Chai <kchai@redhat.com>

tchaikov · 2018-01-26T12:07:59Z

@smithfarm updated and repushed.

smithfarm · 2018-01-26T12:14:36Z

@tchaikov LGTM. Will merge when test build finishes.

* add an option named "MGR_PYTHON_VERSION", so we can build ceph-mgr which use py3 for running plugins * also drop the line to specify the "Python_ADDITIONAL_VERSIONS", because 2.7 is listed by all the the FindPythonInterp and FindPythonLibs in cmake 2.8.12 and up. * use ${MGR_PYTHON_EXECUTABLE} for holding the path to the python interpreter used by mgr. because this variable might be overwritten by "find_package(PythonInterp 2 REQUIRED)" when checking for building env of pybinding for python2. Signed-off-by: Kefu Chai <kchai@redhat.com>

so ceph-mgr can be compiled using py3 Signed-off-by: Kefu Chai <kchai@redhat.com>

tchaikov added the build/ops label Jan 23, 2018

tchaikov requested a review from smithfarm January 23, 2018 08:21

tchaikov added the DNM label Jan 23, 2018

smithfarm reviewed Jan 23, 2018

View reviewed changes

tchaikov force-pushed the wip-cmake-py3 branch from 40bc768 to f707306 Compare January 23, 2018 16:21

smithfarm closed this Jan 23, 2018

smithfarm reopened this Jan 23, 2018

tchaikov force-pushed the wip-cmake-py3 branch 3 times, most recently from 1f3e069 to 5cb94cd Compare January 23, 2018 19:03

tchaikov removed the DNM label Jan 23, 2018

tchaikov requested a review from jcsp January 23, 2018 19:05

smithfarm added the wip-smithfarm-testing label Jan 23, 2018

smithfarm mentioned this pull request Jan 23, 2018

build/ops: rpm: conditionalize Python 2 availability to enable Ceph build on Python 3-only system #20018

Merged

tchaikov force-pushed the wip-cmake-py3 branch from 5cb94cd to 438136d Compare January 24, 2018 02:01

tchaikov force-pushed the wip-cmake-py3 branch from 438136d to 1263b16 Compare January 24, 2018 02:53

smithfarm approved these changes Jan 24, 2018

View reviewed changes

smithfarm removed the wip-smithfarm-testing label Jan 24, 2018

smithfarm added the wip-smithfarm-testing label Jan 24, 2018

cmake: build Boost.python with specified version of python

6f725e7

Signed-off-by: Kefu Chai <kchai@redhat.com>

tchaikov force-pushed the wip-cmake-py3 branch from 1263b16 to 99badd3 Compare January 25, 2018 14:32

jcsp reviewed Jan 25, 2018

View reviewed changes

jcsp approved these changes Jan 25, 2018

View reviewed changes

smithfarm reviewed Jan 25, 2018

View reviewed changes

tchaikov force-pushed the wip-cmake-py3 branch from 99badd3 to e48db01 Compare January 26, 2018 05:01

smithfarm reviewed Jan 26, 2018

View reviewed changes

tchaikov force-pushed the wip-cmake-py3 branch from e48db01 to fd079c4 Compare January 26, 2018 12:07

tchaikov added 2 commits January 26, 2018 20:24

mgr: be compatible with py3

4600307

so ceph-mgr can be compiled using py3 Signed-off-by: Kefu Chai <kchai@redhat.com>

tchaikov force-pushed the wip-cmake-py3 branch from fd079c4 to 4600307 Compare January 26, 2018 12:25

smithfarm merged commit e544f14 into ceph:master Jan 26, 2018

tchaikov deleted the wip-cmake-py3 branch January 26, 2018 14:53

cmake: enabled py3 only build #20064

cmake: enabled py3 only build #20064

Conversation

tchaikov commented Jan 23, 2018

smithfarm commented Jan 23, 2018

smithfarm commented Jan 23, 2018

tchaikov commented Jan 23, 2018

smithfarm commented Jan 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smithfarm commented Jan 23, 2018

smithfarm commented Jan 23, 2018

smithfarm commented Jan 23, 2018

smithfarm commented Jan 23, 2018 • edited

smithfarm commented Jan 23, 2018

smithfarm commented Jan 23, 2018

tchaikov commented Jan 24, 2018 • edited

smithfarm left a comment

Choose a reason for hiding this comment

smithfarm commented Jan 25, 2018 • edited

smithfarm commented Jan 25, 2018 • edited

smithfarm commented Jan 25, 2018

tchaikov commented Jan 25, 2018

jcsp Jan 25, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcsp commented Jan 25, 2018

jcsp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tchaikov commented Jan 26, 2018

smithfarm Jan 26, 2018 • edited

Choose a reason for hiding this comment

tchaikov Jan 26, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tchaikov commented Jan 26, 2018

smithfarm commented Jan 26, 2018

smithfarm commented Jan 23, 2018 •

edited

tchaikov commented Jan 24, 2018 •

edited

smithfarm commented Jan 25, 2018 •

edited

smithfarm commented Jan 25, 2018 •

edited

jcsp Jan 25, 2018 •

edited

smithfarm Jan 26, 2018 •

edited

tchaikov Jan 26, 2018 •

edited