> 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. > > Flavio Percoco wrote: > 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. > > Andrew Stitcher wrote: > I feel strongly about this - program comments *are not* a discussion they > are _documentation_. If it matters who said it then you can use source > control to find out. But there is no good reason why it should matter - > source code must stand alone - it's a form of literature. > > If a discussion is relevant then the comment could include a URL > reference to a permanent web page.
I honestly could argue for hours on this topic but in this case I honestly don't care enough. I'll remove the attributions. > 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. 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. - 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 > >
