qpid-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Andrew Stitcher" <astitc...@apache.org>
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 Thu, 04 Jun 2015 16:33:07 GMT

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


This looks essentially good, but important points below.


proton-c/CMakeLists.txt
<https://reviews.apache.org/r/34809/#comment138685>

    Why is this specific to Linux? Surely if we find tox on whatever platform we can use it.
If we don't find it we can't on whatever plaform.
    
    (This is significant simply because artificial limitations like this make the testing
more limited than possible, and make it harder to port later)



proton-c/CMakeLists.txt
<https://reviews.apache.org/r/34809/#comment138686>

    I'm not sure this message is necessary - we don't usually comment on things not found
if the case is usual.
    
    Certainly don't make it a warning as that interupts an interactive cmake session for no
real reason.



proton-c/bindings/python/setup.py
<https://reviews.apache.org/r/34809/#comment138687>

    It's not necessary to attribute comments - we have source control for that.
    
    It just adds extra noise to read and breaks the reading flow.


- Andrew Stitcher


On June 4, 2015, 4:09 p.m., Kenneth Giusti wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34809/
> -----------------------------------------------------------
> 
> (Updated June 4, 2015, 4:09 p.m.)
> 
> 
> Review request for qpid, Andrew Stitcher, Dominic Evans, Flavio Percoco, 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/CMakeLists.txt 1347f16 
>   proton-c/bindings/python/setup.py 09fa76a 
>   proton-c/bindings/python/setuputils/bundle.py 920744d 
>   proton-c/bindings/python/setuputils/log.py 1e051d4 
>   proton-c/bindings/python/setuputils/misc.py 8978371 
>   proton-c/bindings/python/tox.ini PRE-CREATION 
>   tests/python/proton_tests/common.py 1b8dbdb 
> 
> 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