> 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) > > Flavio Percoco wrote: > 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. > > Andrew Stitcher wrote: > In which case you are tesing the wrong thing - you should be testing > somehow whether setup.py is supported not whether we are on Linux. > > In this case this is a case of understanding the intent of the code as > you read it. > > Flavio Percoco wrote: > We don't need to test whether setup.py is supported, it's always > supported. What we're testing is if this test should/shouldn't be run. If > we're not on Linux, running these tests doesn't make sense because the > setup.py file won't do any compilation from sources but rather expect the lib > to be around. The source compilation *in* the setup.py is enabled just on > Linux.
To Flavio's point - both tests are necessary for now. Eventually we should be able to drop the 'linux' check. We should always be checking for tox, of course. As Flavio pointed out the setup.py will skip attempting to build the proton lib if not linux. Without that check for linux in cmake, we'll end up running the tox tests on the cmake-built proton lib, which means we run the python tests twice on the same library. We should put a comment in the cmake file to make that clear(er). - Kenneth ----------------------------------------------------------- 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 > >
