[cctbxbb] Debian Package

[Baptiste Carvello]

Hi Luc,

I'll discuss mostly my part, and a few general remarks.

Le 09/08/2012 03:35, Luc Bourhis a écrit :
> Radostan Riedel wrote:
>> OK to make that clear a little bit. A few patches are really only for
>> packaging
>> they can't and shouldn't go back upstream.

Well, they shouldn't go into the packages. But if they end up beeing
shared with other distros in the future, it could make sense to host
them in a branch of your source repository. Time will tell.

> So I had missed something indeed! This is my first experience of a Linux
> package in the making, you see. Unfortunately, the both of Baptiste
> and Frederic-Emmanuel answered my criticisms thinking they were aimed at
> your design of the cctbx Debian package. On the contrary, they had to be
> understood in the context of a installation of the cctbx by hand or
> using the package that CCI provides. I am afraid we talked past each
> other here!

I wouldn't say that we talked past each other. Discussing our design
choices helps reduce the "impedance mismatch" between your code and
ours. Also, you know the cctbx code better than us, so your advice is
always helpful.

Regarding installation by hand: the setup.py file could a priori also be
used by end-users to install cctbx to the system python library. I
didn't discuss this possibility because such a system install doesn't
seem to be a supported way to install cctbx.

> Nevertheless, my thanks to Frederic-Emmanuel and Baptiste to elaborate
> on those patches that were only meant for packaging and that I
> criticised most: it is still very useful for us to understand your
> rationales. Some specifics:
>
> [...]

> Baptiste Carvello wrote:
>> The patch 0016-adapt-test_utils-in-libtbx-for-setup_py-test [...]
>>
>> It may be, though, that this can be achieved more cleanly by
>> appropriately reconfiguring the pickled Environment object.
> 
> I would indeed think so but really it is up to you whether you want to
> maintain such a patch or not.

By combining this idea, and the strategy that is used in function
get_module_tests of the new "libtbx/test_utils/parallel.py" module, I
can avoid patching upstream code altogether. That way, all my changes
will be confined in the setup.py and sconsutils.py files, which is much
better. I'll update my patches accordingly in the second half of next week.

> Now about the support of distools. Even though as we both agree this
> code is totally orthogonal to our existing base code, this is no small
> patch and it will go upstream and we will therefore need to maintain it.
> Hence my scrutiny. So thanks for your detailed explanations that will
> save me some time reading your code.

It's a big chunk of code indeed. I tried to keep the different classes
independent, so that they can be reviewed in isolation. The main
difficulty is the need to understand the underlying distutils
architecture: beeing mostly glue code, the classes don't do much by
themselves.

>> Whether this "build_py" class is needed actually depends on you: if the
>> cctbx community is committed to keeping cctbx working also without the
>> "-Qnew" option, we don't need it, which is much better for us.
> 
> This is actually the other way around: we wanted the cctbx to always be
> run with -Qnew and we actually had to fix the code in quite a few places
> to make all tests pass with -Qnew. Having those Python dispatchers in
> the first place, the least intrusive change was definitively to add
> -Qnew. The alternative, adding "from __future__ import division" to
> every single Python module did not appeal to us. Thus that build_py
> class is definitively necessary.

The thing is, right now, most modules in cctbx don't need an additional
__future__ line at all. They run equally well with or without "-Qnew",
for one of 2 reasons:

* either because they already have the __future__ line, probably because
it was already there, and has not been actively stripped,

* or because int division is not used at all. The most common style,
from the little I could see, seems to be using explicit constructs in
all cases, for example "x//2" (explicit integer division) or "x/2."
(explicit floating-point division). This style nicely sidesteps the problem.

If we think that such a style will stay the norm in the future, we don't
need any workaround.

> As I wrote above, the patch is not small and I definitively need to
> spend more time looking into it. And I will therefore come back to you
> with more questions.

Sure. I'll take a few days off starting friday, but I'll be back in the
middle of next week.

Also remember that the "test" part is still a work in progress (so don't
waste too much time on the current implementation), and, as a
consequence, the rest is also quite undertested ;-)

The big design bug I know of is the fact that some of the tests, as well
as the test data, are completely ignored. I won't have time to fix it
before tomorrow.

Cheers,
Baptiste