> On June 4, 2015, 4:33 p.m., Andrew Stitcher wrote: > > This looks essentially good, but important points below.
thanks a lot for the review, Andrew. See my replies below. I'll address the second comment in a bit. > On June 4, 2015, 4:33 p.m., Andrew Stitcher wrote: > > proton-c/bindings/python/setup.py, line 176 > > <https://reviews.apache.org/r/34809/diff/6/?file=979166#file979166line176> > > > > 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. It's a habit I've taken from the current workflow in OpenStack. The extra noise it adds makes it easier to know who to talk to if you've questions. I agree there's source control but it makes you go and run extra commands for something simple. In addition, comments may be moved and that'd count as an addition by someone who might not be related with the comment at all. > On June 4, 2015, 4:33 p.m., Andrew Stitcher wrote: > > proton-c/CMakeLists.txt, line 533 > > <https://reviews.apache.org/r/34809/diff/6/?file=979165#file979165line533> > > > > 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) It's limited because the setup.py currently supports just *linux*. We can't turn the test on just by checking on tox otherwise we'd end up running the test in platforms we're currently not supporting in the setup.py build step. > On June 4, 2015, 4:33 p.m., Andrew Stitcher wrote: > > proton-c/CMakeLists.txt, line 546 > > <https://reviews.apache.org/r/34809/diff/6/?file=979165#file979165line546> > > > > 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. no opinion on this. I'm happy to remove it (although I do appreciate to be notified when a *test* that was supposed to be excuted is not being executed). - Flavio ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34809/#review86638 ----------------------------------------------------------- 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 > >
