qpid-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Gordon Sim" <g...@redhat.com>
Subject Re: Review Request 34809: Flavio's patch to allow python to build and install the libqpid-proton library if not present on the system.
Date Mon, 01 Jun 2015 08:14:48 GMT


> On May 29, 2015, 3:39 p.m., Andrew Stitcher wrote:
> > I unerstand the rationale for not depending on cmake for building proton, but I
think it is a *very* bad idea. As Ken has noted you will then lose whatever cross platform/compiler
abilities the cmake build has. But more importantly we will have a large subset of users that
are running a version of the library that is not tested and is not the same as the developers
are using and testing.
> 
> Flavio Percoco wrote:
>     I agree it's not ideal. I'm not a fan myself of these. However, I do believe the
benefits of this work are bigger than the drawbacks. As a first step forward, I've tried to
be cautions with this change. That is, it'll only be possible to do this on linux and I could
even limit it to 64 bits platforms (although I think it should be fairly simple to add support
for 32 too).
>     
>     Support for future platforms and OSs can be added later on. It'll always be recommended
to use the distro package and the setup.py doesn't build/install anything if there's already
a version of proton-c installed.

Personally I'm in favour of things that make it easier for those interested in trying out
the python library to get hold of it. I agree that by not using cmake you don't get the benefits
it offers in terms of multi- compiler and platform configuration. However ultimately the library
is built by the compiler, not cmake, so for me the issue of it being 'different' is not a
big problem. We do certainly need to figure out how to ensure that the steup.py route is always
tested, but that doesn't seem insurmountable.

As Flavio notes, with this change the library is only built if the right version is not already
available, and building it does not affect any install. This is an additional mechanism for
the convenience of python users, one that didn't exits before, but that doesn't remove any
options or choice from anyone at present.

Ken's points make sense, but otherwise I'm personally happy with this patch. Making proton
easier to consume is very valuable.


- Gordon


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34809/#review85736
-----------------------------------------------------------


On May 29, 2015, 9:09 p.m., Kenneth Giusti wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34809/
> -----------------------------------------------------------
> 
> (Updated May 29, 2015, 9:09 p.m.)
> 
> 
> Review request for qpid, Andrew Stitcher, Dominic Evans, Gordon Sim, and Rafael Schloming.
> 
> 
> Bugs: proton-895
>     https://issues.apache.org/jira/browse/proton-895
> 
> 
> Repository: qpid-proton-git
> 
> 
> Description
> -------
> 
> This change modifies the way setup.py builds and installs the Proton C library should
it not be present on the system.  With this change, setup.py builds the C library directly,
rather than using cmake.  The advantages to this:
> 
> 1) The resultant library is installed in the site-packages directory along with proton.
 This allows non-root users properly install the C librarys when doing a virtualenv or --user
install.
> 2) pip can be used to install/remove the entire proton stack
> 3) only builds the C library - not the whole proton build
> 4) no longer requires cmake, which is not part of a typical python environment
> 
> 
> Diffs
> -----
> 
>   proton-c/bindings/python/setup.py 09fa76a 
>   proton-c/bindings/python/setuputils/misc.py 8978371 
> 
> Diff: https://reviews.apache.org/r/34809/diff/
> 
> 
> Testing
> -------
> 
> All python unit tests pass when run against the bindings and C library installed with
this patch.
> 
> 
> Thanks,
> 
> Kenneth Giusti
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message