You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@qpid.apache.org by Kenneth Giusti <kg...@apache.org> on 2015/05/29 16:26:53 UTC
Review Request 34809: Flavio's patch to allow python to build and
install the libqpid-proton library if not present on the system.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34809/
-----------------------------------------------------------
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 09fa76a0dd8d0f33a92f336c8d441f17748569aa
proton-c/bindings/python/setuputils/misc.py 8978371e8ccf5e8ef5b3aa6ac99592ad8289d336
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
Re: Review Request 34809: Flavio's patch to allow python to build and
install the libqpid-proton library if not present on the system.
Posted by Gordon Sim <gs...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34809/#review86584
-----------------------------------------------------------
Ship it!
My default installed tox was too old apparently, but installing latest into a vritual env and then using that to run make && make test worked with the new tox tests all passing.
I also used the setup.py directly to install into a non-standard location. This works but only if you are in the diretory the setup.py is in (otherwise it can't find the cproton.i for swig). Tested install with examples.
- Gordon Sim
On June 3, 2015, 3:36 p.m., Kenneth Giusti wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34809/
> -----------------------------------------------------------
>
> (Updated June 3, 2015, 3:36 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/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
>
>
Re: Review Request 34809: Flavio's patch to allow python to build and
install the libqpid-proton library if not present on the system.
Posted by Kenneth Giusti <kg...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34809/
-----------------------------------------------------------
(Updated June 3, 2015, 3:36 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 (updated)
-----
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
Re: Review Request 34809: Flavio's patch to allow python to build and
install the libqpid-proton library if not present on the system.
Posted by Kenneth Giusti <kg...@apache.org>.
-----------------------------------------------------------
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 (updated)
-----
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
Re: Review Request 34809: Flavio's patch to allow python to build and
install the libqpid-proton library if not present on the system.
Posted by Kenneth Giusti <kg...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34809/
-----------------------------------------------------------
(Updated May 29, 2015, 5:45 p.m.)
Review request for qpid, Andrew Stitcher, Dominic Evans, Gordon Sim, and Rafael Schloming.
Changes
-------
Flavio's second update
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 (updated)
-----
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
Re: Review Request 34809: Flavio's patch to allow python to build and
install the libqpid-proton library if not present on the system.
Posted by Kenneth Giusti <kg...@apache.org>.
> On May 29, 2015, 2:58 p.m., Kenneth Giusti wrote:
> > proton-c/bindings/python/setup.py, line 231
> > <https://reviews.apache.org/r/34809/diff/1/?file=974311#file974311line231>
> >
> > These flags are platform-specific. I don't think we can hardcode these and expect them to work on different platforms.
We can fix this using distutils - I'll propose these changes in a later patch.
- Kenneth
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34809/#review85729
-----------------------------------------------------------
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
>
>
Re: Review Request 34809: Flavio's patch to allow python to build and
install the libqpid-proton library if not present on the system.
Posted by Flavio Percoco <fl...@gmail.com>.
> On May 29, 2015, 2:58 p.m., Kenneth Giusti wrote:
> > proton-c/bindings/python/setup.py, line 221
> > <https://reviews.apache.org/r/34809/diff/1/?file=974311#file974311line221>
> >
> > This is problematic: we're hardcoding flags for a specific compiler here (gcc). What if clang is used?
> >
> > Is there are way for python to figure out the compiler specific flags?
I can remove these gcc specific flags for sure.
- Flavio
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34809/#review85729
-----------------------------------------------------------
On May 29, 2015, 2:26 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, 2:26 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 09fa76a0dd8d0f33a92f336c8d441f17748569aa
> proton-c/bindings/python/setuputils/misc.py 8978371e8ccf5e8ef5b3aa6ac99592ad8289d336
>
> 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
>
>
Re: Review Request 34809: Flavio's patch to allow python to build and
install the libqpid-proton library if not present on the system.
Posted by Kenneth Giusti <kg...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34809/#review85729
-----------------------------------------------------------
proton-c/bindings/python/setup.py
<https://reviews.apache.org/r/34809/#comment137474>
This is problematic: we're hardcoding flags for a specific compiler here (gcc). What if clang is used?
Is there are way for python to figure out the compiler specific flags?
proton-c/bindings/python/setup.py
<https://reviews.apache.org/r/34809/#comment137475>
These flags are platform-specific. I don't think we can hardcode these and expect them to work on different platforms.
- Kenneth Giusti
On May 29, 2015, 2:26 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, 2:26 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 09fa76a0dd8d0f33a92f336c8d441f17748569aa
> proton-c/bindings/python/setuputils/misc.py 8978371e8ccf5e8ef5b3aa6ac99592ad8289d336
>
> 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
>
>
Re: Review Request 34809: Flavio's patch to allow python to build and
install the libqpid-proton library if not present on the system.
Posted by Gordon Sim <gs...@redhat.com>.
> 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
>
>
Re: Review Request 34809: Flavio's patch to allow python to build and
install the libqpid-proton library if not present on the system.
Posted by Flavio Percoco <fl...@gmail.com>.
> 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.
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.
- Flavio
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34809/#review85736
-----------------------------------------------------------
On May 29, 2015, 2:26 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, 2:26 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 09fa76a0dd8d0f33a92f336c8d441f17748569aa
> proton-c/bindings/python/setuputils/misc.py 8978371e8ccf5e8ef5b3aa6ac99592ad8289d336
>
> 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
>
>
Re: Review Request 34809: Flavio's patch to allow python to build and
install the libqpid-proton library if not present on the system.
Posted by Kenneth Giusti <kg...@apache.org>.
> 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.
>
> Gordon Sim wrote:
> 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.
I'd +1 this patch if we could address the test coverage problem. Without thinking through the details, might we be able to use tox and/or virtualenv to:
1) force setup.py to install via the 'bundle' method, and
2) run the proton-tests python tests using the built/installed library?
That would essentially exercise exactly the deployment scenario we're trying to address with this patch.
The biggest gotcha here is that, in the case of a developer building out of their source repo, we want setup.py to use the local source as the 'bundle' so we can test the developer's work. We probably only want setup.py to download the proton-c sources when it's being installed via the pypi python bindings package.
- Kenneth
-----------------------------------------------------------
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
>
>
Re: Review Request 34809: Flavio's patch to allow python to build and
install the libqpid-proton library if not present on the system.
Posted by Flavio Percoco <fl...@gmail.com>.
> 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.
>
> Gordon Sim wrote:
> 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.
>
> Kenneth Giusti wrote:
> I'd +1 this patch if we could address the test coverage problem. Without thinking through the details, might we be able to use tox and/or virtualenv to:
>
> 1) force setup.py to install via the 'bundle' method, and
> 2) run the proton-tests python tests using the built/installed library?
>
> That would essentially exercise exactly the deployment scenario we're trying to address with this patch.
>
> The biggest gotcha here is that, in the case of a developer building out of their source repo, we want setup.py to use the local source as the 'bundle' so we can test the developer's work. We probably only want setup.py to download the proton-c sources when it's being installed via the pypi python bindings package.
Thanks for y'all for your feedback.
Kenneth, I think that's actually a great thing to have and we can make it work with the existing test framework used for proton-c.
Note that just like the setup.py itself, this test will be enabled conditionally and therefore, it'll be executed just in the platforms supported by the setup.py script itself.
I'll be sending a patch with those tests soon.
- Flavio
-----------------------------------------------------------
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
>
>
Re: Review Request 34809: Flavio's patch to allow python to build and
install the libqpid-proton library if not present on the system.
Posted by Andrew Stitcher <as...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34809/#review85736
-----------------------------------------------------------
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.
- Andrew Stitcher
On May 29, 2015, 2:26 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, 2:26 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 09fa76a0dd8d0f33a92f336c8d441f17748569aa
> proton-c/bindings/python/setuputils/misc.py 8978371e8ccf5e8ef5b3aa6ac99592ad8289d336
>
> 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
>
>