Bazaar on IronPython

[Martin Pool]

2009/6/27 Martin (gzlist) <gzlist at googlemail.com>:
> In short, see the attached patch for getting bazaar to run on
> IronPython. It only passes a subset of the tests, but can do some of
> the basics like creating a new repo, adding and diffing, and
> committing the changes. This is not intended for merging, many of the
> changes are addressing interpreter bugs and differences rather than
> problems with the code. However there are a number of issues raised
> that might be of interest to bazaar developers.

That sounds pretty interesting.  If you want to do a separate patch
that is intended for merging, even if it doesn't get everything
working but just makes more things work then we could certainly look
at merging it.

>
> Having nibbled round the edges of the bzrlib.osutils module, I wanted
> a task that would serve as an excuse to look over the platform related
> code as a whole. I'd downloaded IronPython some months back and not
> had cause to play with it, and seeing that Adrian Wilkins investigated
> this a year ago and wrote up some helpful tips, it seemed achievable.
> <http://bazaar-vcs.org/IronPython>
> <http://ironpython-urls.blogspot.com/2008/03/bazaar-on-ironpython.html>
>
> My basic strategy was to get selftest working then deal with
> categories of test failures. I mostly used IronPython 2.0.0 though I
> tried the first 2.6 beta towards the end, which threw up a few extra
> issues, but may have been a better choice to start as it's less
> compiling-the-world slow on every invocation. There's still work to be
> done to make bazaar generally usable under IronPython, and fair bit of
> low hanging fruit (for instance I left warnings about terminal
> encodings untouched as a useful progress indicator).
>
> Some misc observations from the process:
>
> Bazaar has a lot of platform switching code in various different
> places. I get a strong impression that anything touching sys.platform
> or errno should be migrated to osutils.

I would probably agree with that, pending a look at the particular
cases.  One other that comes to mind is that tests may also want to
switch on the platform, but even there I'd say that in general tests
should be depending on Feature classes present on the platform or not.

We could eventually potentially add a test_source saying you must not
look at sys.platform other than in particular places.

> Using ref-counted RAII is convenient. To change everything to
> finally-close would be too much pain for no actual benefit.

Well, I agree it's more convenient, but I don't think it's a good
tradeoff.  The problems with running code from __del__ are: exceptions
raised there are lost, you don't know when precisely it will run, it
may not run at all before the process exits, because of the preceding
points it's hard to test, it may impede garbage collection, and
perhaps more.  The net effect is that you cannot use it for anything
that really has to be done, like releasing a lock.  Just about the
only thing we use it for at the moment is giving a warning that the
object was not closed, and I think even that is questionable.  (See
https://bugs.edge.launchpad.net/bzr/+bug/391024)

>
> Required modules - I could get away with moving subprocess and bz2 to
> lazy import, but too much of the test suite mechanics depended on
> zlib. There's an emulation binary created by Jeff Hardy that resolved
> that, though it has various problems (noted in the patch near the
> workarounds).
> <http://jdhardy.blogspot.com/2008/12/solving-zlib-problem-ironpythonzlib.html>
> <http://cid-414fa1a9bd174b4b.skydrive.live.com/self.aspx/Public/IronPython.Zlib.zip>

We do use zlib fairly extensively, so we do need something.  Perhaps
we can add a layer that will work on DeflateStream.

> The combination of bazaar trying to pretend that everything is a UTF-8
> byte string and IronPython trying to pretend the str type is unicode
> is recipe for trouble.  I think bazaar should give here, and sort out
> a better set of idioms for working with both binary data and
> non-english text - it'll be needed for Python 3 anyway.
> <http://www.infoq.com/news/2007/06/IronPython-STR>
> <http://www.smallshire.org.uk/sufficientlysmall/2009/06/18/string-compatibility-between-python-implementations/>

It's not so much that we're trying to pretend everything is UTF-8 as
that we make a lot of use of byte strings, either utf8 or otherwise.
In python 2.x, the byte string type is called 'str'.  I agree that
distinguishing it is better.  I guess we could use the name 'bytes'
here and on python<=2.5 just do 'bytes = str' at startup.  It would be
more clear.  I suspect that's not enough though.

>
> IronPython tries in some places to be helpful by stubbing in various
> methods and raising NotImplementedError-s from them - but didn't get
> the signatures right, so you get some completely different cryptic
> exception instead.
>
> This line at the bottom of bzrlib.builtins:
>    from bzrlib.foreign import cmd_dpush
> pulls in a bunch of extra imports, and makes a difference of about a
> tenth of a second and a megabyte of disk read to `bzr rocks` on my
> machine. Or twenty four seconds for IronPython 2.0.0...

That seems like a bug; it should be a lazy registration instead.

> Likewise, it seems to make inspect_for_copy.py pointless. Is there any
> ongoing auditing of perf hacks like the lazy imports and regexps and
> this to ensure they remain net wins?

Just the sort of thing you're doing here, people occasionally looking
into the performance for things that look bad.  It seems like we
should be able to write a script that makes assertions about what is
loaded and then checks both that it is reasonable and that it's
consistent with what's supposed to be lazy-loaded.  But that's not
been done afaik.

> Overall, it was quite impressive how little needed altering, though
> the path to discovering those small changes was often winding, even
> the root cause was the same as a previously handled issue.

I think that's a good reason to get a mergeable patch merged, so that
next time you or someone comes to it they won't be starting from
scratch.

I haven't read your patch yet, let me know if you actually want a review.

-- 
Martin <http://launchpad.net/~mbp/>