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

Make Rados ready for PyPI #9332

Closed
wants to merge 789 commits into from
Closed

Make Rados ready for PyPI #9332

wants to merge 789 commits into from

Conversation

onyb
Copy link
Contributor

@onyb onyb commented May 25, 2016

Test package is hosted on http://testpypi.python.org/pypi/rados

Steps to build from source:

  • First install the development headers and libraries of Python and Rados.

    $ sudo apt-get install python-dev librados-dev
    
  • Install Cython

    $ sudo pip install cython
    
  • Install Rados bindings

    $ cd src/pybind/rados
    $ sudo python setup.py install
    
  • (optional) Install Rados bindings from pre-cythonized source
    You don't need to have Cython installed in this case.

    $ cd src/pybind/rados
    $ sudo python setup.py install --without-cython
    
  • Building Rados bindings from Ceph top-level

    $ PYTHON=python3 ./configure
    $ make
    

Note that virtualenv and pyvenv are also supported.

Steps to run the test package:

$ sudo pip install -i https://testpypi.python.org/pypi rados

@ktdreyer
Copy link
Member

Want to link to http://tracker.ceph.com/issues/5900 ?

@jcsp
Copy link
Contributor

jcsp commented May 26, 2016

Sorry if this has already been discussed, but why is it useful to have our rados modules on pypi? They are not versioned independently of librados.so, so unless someone happens to have exactly the same versions of both, things are liable to not work.

Given that someone will always need native (librados) packages when using the python modules, what is the advantage of getting the python modules from pypi instead of getting the python modules from the packages that we provide?

@onyb
Copy link
Contributor Author

onyb commented May 26, 2016

@jcsp: I am not very sure how the Debian package python-rados has been packaged, but I think it doesn't contain the new Cython bindings, and installs only for Python 2.7.

Advantage of using pip is that we can ship Cython extensions which will be cythonized and compiled according to the Python version pip was launched with. This is also important because bindings compiled on Python 2 won't work on Python 3, and vice-versa. Another reason to go for pip is that it allows installing rados inside virtual environments.

Maybe @jdurgin will have something to add.

@onyb
Copy link
Contributor Author

onyb commented May 26, 2016

@ktdreyer Updated issue #5900 with a link to this PR.

@jdurgin
Copy link
Member

jdurgin commented May 27, 2016

@jcsp the version requirement isn't exact, as-is older python bindings will continue to work with new libraries, they'll only fail when newer python bindings are used with older libraries which don't have new functions that the python bindings use.

The drive for this is to play nicer with python packaging for developers, and like @onyb mentioned using different versions of python or the bindings is one aspect. It allows virtualenvs with no system site packages. Compiling just the c generated by cython isn't expensive, so it seems like a good tradeoff to me, rather than trying to bundle librados or compile it in the python package.

@jdurgin
Copy link
Member

jdurgin commented May 27, 2016

Is it necessary to commit the generated .c file? Could that be created by the usual build process before creating the python package?

'License :: OSI Approved :: GNU Lesser General Public License v2 or later (LGPLv2+)',
'Operating System :: POSIX :: Linux',
'Programming Language :: Cython',
'Programming Language :: Python :: 2.6',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can leave off 2.6 - no distros ceph supports use it, and this setup.py won't work with it (subprocess.check_output() isn't there until 2.7).

Is it necessary to specify all the minor versions rather than just python 2 and python 3?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I idea was to be explicit about the versions supported. I kept Python 2.6+ and 3.2+ since it was supported by the bindings, but it is seems logical to drop 2.6, 3.2, and 3.3.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On 05/27/2016 02:50 AM, Anirudha Bose wrote:

In src/pybind/rados/setup.py
#9332 (comment):

         )
  • ], build_dir=os.environ.get("CYTHON_BUILD_DIR", None)),
  • cmdclass={
  •    "egg_info": EggInfoCommand,
    
  • },
  •    ], build_dir=os.environ.get("CYTHON_BUILD_DIR", None)
    
  • ),
  • classifiers=[
  •    'Intended Audience :: Developers',
    
  •    'Intended Audience :: System Administrators',
    
  •    'License :: OSI Approved :: GNU Lesser General Public License v2 or later (LGPLv2+)',
    
  •    'Operating System :: POSIX :: Linux',
    
  •    'Programming Language :: Cython',
    
  •    'Programming Language :: Python :: 2.6',
    

I idea was to be explicit about the versions supported. I kept Python
2.6+ and 3.2+ since it was supported by the bindings, but it is seems
logical to drop 2.6, 3.2, and 3.3.

Sounds good

@jdurgin
Copy link
Member

jdurgin commented May 27, 2016

@alfredodeza perhaps you'd like to take a look at this too

@onyb
Copy link
Contributor Author

onyb commented May 27, 2016

@jdurgin The motivation behind including the cythonized file is to allow users to do a python setup.py install --without-cython if they don't want to have Cython as a dependency. Said that, not committing the file is an option, but will only work for pip install, and not python setup.py install.

I can push a commit to exclude rados.c from source tree, and only include it in the PyPI source distribution. I will also drop the --without-cython option, and instead use the already cythonized .c file only if Cython is not available.

@jdurgin
Copy link
Member

jdurgin commented May 27, 2016

@onyb that sounds good wrt to rados.c

@jdurgin
Copy link
Member

jdurgin commented Jun 9, 2016

built nicely, and it's qutie easy to reconfigure + rebuild for a new python version. I pushed this as wip-rados-pypi so we can see how it builds on different platforms (http://ceph.com/gitbuilder.cgi)

@jdurgin
Copy link
Member

jdurgin commented Jun 9, 2016

We will want to make this work with cmake too (I think that could be a later PR though)
@alimaredia @tchaikov any comments on this?

@onyb
Copy link
Contributor Author

onyb commented Jun 9, 2016

@jdurgin I see builds in Gitbuilder fail with an error: option --single-version-externally-managed not recognized. The reason is that the setup.py which uses only distutils (not setuptools), doesn't accept that option. Switching back to using setuptools should fix the problem.

yuyuyu101 and others added 10 commits June 11, 2016 13:39
buffer: handle integer underflow in iterator::copy(large_int, dest)

Reviewed-by: Sage Weil <sage@redhat.com>
mon: fix typo of 'ceph auth rm'

Reviewed-by: xie xingguo <xie.xingguo@zte.com.cn>
osd/OSD.h: change iterator to const_iterator

Reviewed-by: Kefu Chai <kchai@redhat.com>
osd: sparse_read offset may not be zero for ecpool

Reviewed-by: Kefu Chai <kchai@redhat.com>
python: Pass prefix/sbindir from autoconf to distutils.

Reviewed-by: Kefu Chai <kchai@redhat.com>
otherwise it looks in $PATH for the script

Signed-off-by: Mykola Golub <mgolub@mirantis.com>
The mdcache->request_start() method may lose the forward race
against a slave and thus may either drop the last reference of the
passed in message or push the message into the waiting_for_finish
list for a later retry.

In either case we shall stall and exit, otherwise we'll access
violation especifally for the former case(as we drop the last
reference of "req" and it shall be destructed by now).

Signed-off-by: xie xingguo <xie.xingguo@zte.com.cn>
Respond_to_request will do cleanup jobs, thus we shall
stall and exit under this case.

Signed-off-by: xie xingguo <xie.xingguo@zte.com.cn>
It's better to respond and do the cleanup jobs anyway.

Signed-off-by: xie xingguo <xie.xingguo@zte.com.cn>
rgw: check for -ERR_NOT_MODIFIED in rgw_rest_s3.cc
Reviewed-by: Orit Wasserman <owasserm@redhat.com>
@alimaredia
Copy link
Contributor

@tchaikov will you take care of the ramifications to CMake of this PR?

alimaredia and others added 23 commits June 17, 2016 13:51
cmake: add ceph_test_* used by qa units and ceph-qa-suite tasks

Reviewed-by: Ali Maredia <amaredia@redhat.com>
Update git rev parsing in CMake

Reviewed-by: Ali Maredia <amaredia@redhat.com>
cmake: add arguments to cmake invocation to support cache configuration

Reviewed-by: Ali Maredia <amaredia@redhat.com>
rgw: fix deadlock in RGWHTTPManager when HAVE_CURL_MULTI_WAIT=0
Reviewed-by: Orit Wasserman <owasserm@redhat.com>
added -f flag for formatting to rados command line tool

Reviewed-by: Samuel Just <sjust@redhat.com>
mention change of owner for /var/log/ceph
test/rgw/test_http_manager.cc: In member function ‘virtual void HTTPManager_SignalThread_Test::TestBody()’:
test/rgw/test_http_manager.cc:40:23: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
   for (int i = 0; i < num_requests; i++) {
                       ^

Signed-off-by: Yan Jun <yan.jun8@zte.com.cn>
... to the point where they are first used.

Signed-off-by: Michal Jarzabek <stiopa@gmail.com>
rgw: kill compiling warning

Reviewed-by: Casey Bodley <cbodley@redhat.com>
…geScope

common/address_helper.cc:move variables closer...

Reviewed-by: Kefu Chai <kchai@redhat.com>
cmake: update description for cmake module Findkeyutils.cmake

Reviewed-by: Ali Maredia <amaredia@redhat.com>
Reviewed-by: Kefu Chai <kchai@redhat.com>
os/bluestore: remove unused bluestore fields

Reviewed-by: Allen Samuels <allen.samuels@sandisk.com>
os/bluestore: fix improper blob's csum visualization.

Reviewed-by: Sage Weil <sage@redhat.com>
os/bluestore: reduce bluestore blob

Reviewed-by: Sage Weil <sage@redhat.com>
* Uses the PYTHON environment variable allowed by configure scripts
* Supports running inside Python virtual environments, in which case,
  setting the PYTHON variable is not required.
* Example: $ PYTHON=python3.5 ./configure

Signed-off-by: Anirudha Bose <ani07nov@gmail.com>
Detects appropriate Python CFLAGS/LDFLAGS for Cythonizing

Signed-off-by: Anirudha Bose <ani07nov@gmail.com>
Signed-off-by: Anirudha Bose <ani07nov@gmail.com>
Pre-cythonized file rados.c is now included in the Git tree

Signed-off-by: Anirudha Bose <ani07nov@gmail.com>
Signed-off-by: Anirudha Bose <ani07nov@gmail.com>
Signed-off-by: Anirudha Bose <ani07nov@gmail.com>
The CFLAGS and LDFLAGS were not visible to the dummy librados
program when run from top-level make

Signed-off-by: Anirudha Bose <ani07nov@gmail.com>
* Remove rados.c from Git tree
* Remove Python 2.6, 3.2, 3.3 from classifiers
* Export CEPH_LIB_DIR to setup.py files in pybind

Signed-off-by: Anirudha Bose <ani07nov@gmail.com>
@onyb onyb closed this Jun 19, 2016
@onyb onyb deleted the wip-rados-pypi branch June 19, 2016 14:17
@tchaikov
Copy link
Contributor

@alimaredia i will.

@onyb onyb mentioned this pull request Jun 20, 2016
@onyb
Copy link
Contributor Author

onyb commented Jun 20, 2016

Please head over to pull request #9833

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet