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 19:33:04 GMT


> 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.
> 
> Kenneth Giusti wrote:
>     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).

I understand the aim of the test, but its current implentation is poor engineering. It is
not testing whether setup.py will attempt to make the proton lib, but rather something else
(that currently indirectly causes this).

Under future maintenance this may change, but someone reading this code will think it is somehow
a platform specific thing - there should be a properly named boolean here which describes
the real condition. 

[I'm finding it a bit discouraging that I should have to explain elementary software engineering
practices here]


> 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.
> 
> Flavio Percoco wrote:
>     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).
> 
> Kenneth Giusti wrote:
>     I'd rather you keep it, and simply make it informational rather than a warning. 
 This message is issued if the python bindings are being tested, but in the case tox is not
available then we're not only missing the test of the setup.py-installed bindings, but in
the future we'll also be skipping any multi-version python testing (py3 for example).
>     
>     I'd like to get those devs who are interested in python bindings to install the de
facto standard python testing tools.  Hopefully this little message will nudge some to do
so.

I'm ok with this - although everyone thinks that their messages are special snowflakes!


> 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.
> 
> Flavio Percoco wrote:
>     I honestly could argue for hours on this topic but in this case I honestly don't
care enough. I'll remove the attributions.

Thank you


- Andrew


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


On June 4, 2015, 6:07 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, 6:07 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