Re: [PATCH v1 0/6] tools/ocaml/xenstored: simplify code

[Edwin Torok <edvin.torok@xxxxxxxxxx>]

On Thu, 2020-08-27 at 09:41 +0000, Wei Liu wrote:
> On Tue, Aug 18, 2020 at 01:40:18PM +0100, Andrew Cooper wrote:
> > On 18/08/2020 10:25, Christian Lindig wrote:
> > > I see little reason to support old OCaml releases and requiring
> > > OCaml 4.06 would be fine with me but I assume that the project
> > > might have its own ideas about this.
> > > 
> > > ________________________________________
> > > From: Edwin Torok
> > > Sent: 18 August 2020 08:28
> > > To: Christian Lindig; xen-devel@xxxxxxxxxxxxxxxxxxxx
> > > Cc: Ian Jackson; dave@xxxxxxxxxx; wl@xxxxxxx
> > > Subject: Re: [PATCH v1 0/6] tools/ocaml/xenstored: simplify code
> > > 
> > > On Mon, 2020-08-17 at 14:56 +0200, Christian Lindig wrote:
> > > > This all looks good - I left a small comment on one of the
> > > > patches
> > > > and I agree that this needs testing. I also wonder about
> > > > compatibility with earlier OCaml releases that we support but I
> > > > see
> > > > no real obstacles.
> > > > 
> > > I've developed the series using OCaml 4.08.1. I think the newest
> > > feature I used was Map.update (OCaml 4.06, nearly 3 years ago).
> > > Looking through https://repology.org/project/ocaml/versions I'm
> > > not
> > > sure if we can require more than 4.05 though.
> > > The README in Xen doesn't specify a minimum version, but
> > > configure
> > > checks for >=4.02.
> > > 
> > > I can try to backport my series to OCaml 4.05 (to use
> > > Map.find_opt
> > > instead of Map.update) and update the configure check to require
> > > 4.05.
> > > It would be possible to backport even further to 4.02 by
> > > introducing
> > > additional inefficiencies (Map.mem + Map.find would traverse the
> > > map
> > > twice, and Map.find on its own would raise an exception on Not
> > > found,
> > > which is more costly than returning None in Map.find_opt), I'd
> > > avoid
> > > doing that.
> > > 
> > > Xen's CI from automation might need some updates to use latest
> > > stable
> > > versions:
> > > * Fedora 29 is EOL, should use at least Fedora 31
> > > * Debian Jessie is EOL. Stretch is present, but Buster is missing
> > 
> > We're working on the CI loop.
> > 
> > As maintainer, it is ultimately Christian's choice to as to if/when
> > to
> > bump the minimum versions.
> > 
> > 
> > As a general rule, we don't want to be sufficiently bleeding edge
> > to
> > rule out in-use distros.  I have no idea if 4.06 is ok there, or
> > whether
> > it is too new.  Then again, the Ocaml components are strictly
> > optional
> > so it is perhaps less important.
> > 
> > Whatever happens WRT version, the configure change should occur
> > before
> > changes in the code which would fail on older versions.
> 
> Yes I would like to see the bump happen before applying a version of
> this series too.

I have a branch that removes the requirement on 4.06 from the series,
and then have a separate one that bumps to 4.06 and removes the
redundant and inefficient workaround.
In the end I only had to backport 2 functions: Map.find_opt and
Map.update.

Will send out the updated series for review once I've done some more
testing on it.

Best regards,
--Edwin

> 
> Wei.
> 
> > ~Andrew