1203266 - Extract the build system's view of test metadata from the emitter and data modules to make more of it available earlier in the build

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Chris Manchester (limited bugmail, email directly)]

Bug 1184405 suggests that some view of the build system's data on tests should be readily available without a build (and without hacks to run part of the emitter outside of a build). This could mean reading test manifests as a part of the build reader, extracting more parts of the emitter concerned with reading test manifests, or even incorporating the manifestparser into the build system in some way.

Comment 1

[Chris Manchester (limited bugmail, email directly)]

Attachment: MozReview Request: [mq]: dummy1

Bug 1203266 - Read test manifests in the reader instead of the emitter to make them available earlier in the build. r=gps

Comment 2

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8669165 [details]
MozReview Request: [mq]: dummy1

f? only for now because I'm a bit unsure of the direction here (although it is effective).

Running mach build-backend a few times, this consistently moves 200ms of reported time from the "Processed into <count> build config descriptors in" section to the "Finished reading <count> moz.build files in" section, with the overall times remaining comparable.

If this is generally on the right track I'll extend it to the other manifest types and thread it through the files_info code.

Comment 3

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/21135/#review19029

Yes, moving things from emitter to reader is something we're generally interested in. And this patch does it well.

Please note the special Mercurial-based reading mode for moz.build files: we support reading and evaluating moz.build files directly from a Mercurial repository, without any working copy / checkout. We had to change a number of places that assumed we were dealing with files, not file handles. There is an abstracted I/O layer that you should go through to ensure the test manifest parser can read content from Mercurial.

On that front, reading test manifests at moz.build time does add some overhead. This is one place where the separation between reader and emitter is beneficial. I'd be much happier if test manifests didn't take as long to read. Perhaps you could profile that while you are mucking about with test manifest handling :)

::: python/mozbuild/mozbuild/frontend/context.py:539
(Diff revision 1)
> +    class ListWithAction(ContextDerivedValue, List):

This feels like something that should be inthe base class. Perhaps you can pass a callable to its __init__ that gets called when defined and gives the opportunity to mutate the value?

::: python/mozbuild/mozbuild/frontend/context.py:569
(Diff revision 1)
> +ManifestparserTestManifestList = OrderedListWithActionFactory(read_single_manifest)

This is elegant and I like it!

Comment 4

[Chris Manchester (limited bugmail, email directly)]

(In reply to Gregory Szorc [:gps] from comment #3)
> 
> On that front, reading test manifests at moz.build time does add some
> overhead. This is one place where the separation between reader and emitter
> is beneficial. I'd be much happier if test manifests didn't take as long to
> read. Perhaps you could profile that while you are mucking about with test
> manifest handling :)

I checked into this, only prominent thing in the profile was a call to normpath taking about 25% of the time spent reading manifests. Unfortunately, this is called for every test path in every manifest, but removing it would certainly change output.

On my laptop reading all manifestparser manifests takes around 190ms (~3% of |./mach build-backend|).

Comment 5

[Mike Hommey [:glandium]]

See bug 1191575 for some analysis about the test manifest parsing thing

Comment 6

[Mike Hommey [:glandium]]

(In reply to Mike Hommey [:glandium] from comment #5)
> See bug 1191575 for some analysis about the test manifest parsing thing

comments 5 to 8.

Comment 7

[Chris Manchester (limited bugmail, email directly)]

Attachment: PR to allow passing a file-like object to the wpt manifest reader

This is comment 3 for wpt manifests, the commit message has a summary.

Comment 8

[Chris Manchester (limited bugmail, email directly)]

(In reply to Mike Hommey [:glandium] from comment #6)
> (In reply to Mike Hommey [:glandium] from comment #5)
> > See bug 1191575 for some analysis about the test manifest parsing thing
> 
> comments 5 to 8.

I don't see a reason not to use the "if '..' in path" optimization mentioned there (it will change the result if test paths have "//" or "/./" in them, but I don't think that matters).

Comment 9

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8669165 [details]
MozReview Request: [mq]: dummy1

Bug 1203266 - Don't call normpath in the manifestparser on paths that don't contain '..'. r=ahal

Profiling the manifestparser revealed that 25% of its total time is spent in a single
call to os.path.normpath, called on each test path in each manifest. For the manifests
in mozilla-central, these calls almost always return their input. To save time, this
patch modifies the manifestparser to not call normpath on paths that don't contain
'..'. This will change the parsed result of manifests containing no-op path
components ('//' or '/./') to include those components in their output.

Comment 10

[Chris Manchester (limited bugmail, email directly)]

Attachment: MozReview Request: [mq]: dummy2

Bug 1203266 - Optionally read manifestparser manifests with a Finder class. r=ahal

We need to go through an abstraction layer when manifestparser manifests
are read as a part of reading moz.build files in case moz.build reading
is using mercurial as its filesystem. This adds an optional finder member,
which will be used if present for IO that happens when reading manifests,
This needs to be optional, because the manifestparser can be used as a
standalone package that isn't distributed with mozpack.

Comment 11

[Chris Manchester (limited bugmail, email directly)]

Attachment: MozReview Request: Bug 1203266 - Allow reading reftest manifests from a provided finder. r=gps

Bug 1203266 - Allow reading reftest manifests from a provided finder. r=gps

Comment 12

[Chris Manchester (limited bugmail, email directly)]

Attachment: MozReview Request: Bug 1203266 - Add a list variant to mozbuild.util that allows pre-processing list items with a provided callable. r=gps

Bug 1203266 - Add a list variant to mozbuild.util that allows pre-processing list items with a provided callable. r=gps

Comment 13

[Chris Manchester (limited bugmail, email directly)]

Attachment: MozReview Request: Bug 1203266 - Read test manifests in the reader instead of the emitter to make them available earlier in the build. r=gps

Bug 1203266 - Read test manifests in the reader instead of the emitter to make them available earlier in the build. r=gps

Comment 14

[Chris Manchester (limited bugmail, email directly)]

Attachment: MozReview Request: Bug 1203266 - Remove uses of the emitter in files_info now that they're no longer needed. r=gps

Bug 1203266 - Remove uses of the emitter in files_info now that they're no longer needed. r=gps

Comment 15

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8672054 [details] [review]
PR to allow passing a file-like object to the wpt manifest reader

This has been merged, thank you.

Comment 16

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/21135/#review19537

::: testing/mozbase/manifestparser/manifestparser/manifestparser.py:157
(Diff revision 2)
>                  path = normalize_path(path)

It appears we're doing path normalization up here. Could we not perform the os.path.join() above and always do the normalization below? That should make this extra normpath() unnecessary and have the same effect as this patch.

Comment 17

[Gregory Szorc [:gps]]

Comment on attachment 8672060 [details]
MozReview Request: Bug 1203266 - Allow reading reftest manifests from a provided finder. r=gps

https://reviewboard.mozilla.org/r/21509/#review19539

::: layout/tools/reftest/reftest/__init__.py:61
(Diff revision 1)
> +        if self.finder:
> +            lines = self.finder.get(path).read().splitlines()
> +        else:
> -        with open(path, 'r') as fh:
> +            with open(path, 'r') as fh:
> +                lines = fh.read().splitlines()

A subtle, possibly-not-relevant difference in this code is that `for line in fh` returns the line with the newline char(s) and `splitline()` strips the newline char(s).

Since we're doing a line.strip() below, it almost certainly doesn't matter.

::: layout/tools/reftest/reftest/__init__.py:62
(Diff revision 1)
> +            lines = self.finder.get(path).read().splitlines()

I don't like scope bloating, but given this and the previous patch, we may want to implement readline() on Finder instances.

Comment 18

[Gregory Szorc [:gps]]

Comment on attachment 8672072 [details]
MozReview Request: Bug 1203266 - Add a list variant to mozbuild.util that allows pre-processing list items with a provided callable. r=gps

https://reviewboard.mozilla.org/r/21667/#review19541

::: python/mozbuild/mozbuild/util.py:266
(Diff revision 1)
> -    def __init__(self, iterable=[]):
> +    def __init__(self, iterable=[], **kwargs):

While you are here, using [] as default arguments is bad. See http://docs.python-guide.org/en/latest/writing/gotchas/#mutable-default-arguments.

We're likely not running into any problems because we're not relying on the default value in any callers. Still, this is a giant land mine waiting to strike.

::: python/mozbuild/mozbuild/util.py:385
(Diff revision 1)
> +class ListWithActionMixin(object):

Please add a docstring.

::: python/mozbuild/mozbuild/util.py:386
(Diff revision 1)
> +    def __init__(self, iterable=[], action=None):

action appears to be required. So no need to use a default value.

::: python/mozbuild/mozbuild/util.py:389
(Diff revision 1)
> +        return super(ListWithActionMixin, self).__init__(iterable)

You don't need to return anything from __init__ functions. Not sure why this is done in other classes. Please don't cargo cult it.

(__new__ is the function that needs to return something. In fact, when you use __new__, a non-None return by __init__ will raise TypeError!)

::: python/mozbuild/mozbuild/util.py:401
(Diff revision 1)
> +        import pdb; pdb.set_trace()

pdb?!

Comment 19

[Chris Manchester (limited bugmail, email directly)]

https://reviewboard.mozilla.org/r/21135/#review19537

> It appears we're doing path normalization up here. Could we not perform the os.path.join() above and always do the normalization below? That should make this extra normpath() unnecessary and have the same effect as this patch.

The "normalize_path" function in the manifestparser only replaces separators, it doesn't collapse ..'s (why that is, I don't know).

Comment 20

[Chris Manchester (limited bugmail, email directly)]

https://reviewboard.mozilla.org/r/21667/#review19541

> action appears to be required. So no need to use a default value.

But changing the order of parameters would do weirdness. I'll raise if it isn't provided.

> pdb?!

Oh, right. This method (and StrictOrderingOnAppendListMixin's __add__) are dead code, because ListMixin's __add__ never calls super.

Comment 21

[Andrew Halberstadt [:ahal]]

Comment on attachment 8669165 [details]
MozReview Request: [mq]: dummy1

https://reviewboard.mozilla.org/r/21135/#review19605

Comment 22

[Andrew Halberstadt [:ahal]]

Comment on attachment 8672059 [details]
MozReview Request: [mq]: dummy2

https://reviewboard.mozilla.org/r/21507/#review19607

Looks good to me, but have a few questions and doc cleanup before giving an r+

::: testing/mozbase/manifestparser/manifestparser/ini.py:32
(Diff revision 1)
> -    for (linenum, line) in enumerate(fp.readlines(), start=1):
> +    for (linenum, line) in enumerate(fp.read().splitlines(), start=1):

Why is read() + splitlines() better than readlines() ?

::: testing/mozbase/manifestparser/manifestparser/manifestparser.py:47
(Diff revision 1)
> -    def __init__(self, manifests=(), defaults=None, strict=True, rootdir=None):
> +    def __init__(self, manifests=(), defaults=None, strict=True, rootdir=None,
> +                 finder=None):

Would you mind adding a docstring here while you're changing the signature anyway?

::: testing/mozbase/manifestparser/manifestparser/manifestparser.py:100
(Diff revision 1)
> +            # If we're using mercurial as our filesystem via a finder
> +            # during manifest reading, the getcwd() calls that happen
> +            # with abspath calls will not be meaningful, so absolute
> +            # paths are required.

This comment doesn't make sense to me. Also what does mercurial have to do with anything?

::: testing/mozbase/manifestparser/manifestparser/manifestparser.py:107
(Diff revision 1)
> -            fp = open(filename)
> +            fp = self.get_fp(filename)

nit: if this is the only place it's called, might as well inline 'get_fp'.

Comment 23

[Chris Manchester (limited bugmail, email directly)]

https://reviewboard.mozilla.org/r/21507/#review19607

> Why is read() + splitlines() better than readlines() ?

"fp" is going to be a File object from python/mozbuild/mozpack/files.py, and they don't implement readlines() (although I may just fix that).

> Would you mind adding a docstring here while you're changing the signature anyway?

I'll try!

> This comment doesn't make sense to me. Also what does mercurial have to do with anything?

One of the variants of FileFinder is MercurialFinder, which backs reads with hg, so I don't think getcwd is going to be what we want.

Comment 24

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8669165 [details]
MozReview Request: [mq]: dummy1

Bug 1203266 - Don't call normpath in the manifestparser on paths that don't contain '..'. r=ahal

Profiling the manifestparser revealed that 25% of its total time is spent in a single
call to os.path.normpath, called on each test path in each manifest. For the manifests
in mozilla-central, these calls almost always return their input. To save time, this
patch modifies the manifestparser to not call normpath on paths that don't contain
'..'. This will change the parsed result of manifests containing no-op path
components ('//' or '/./') to include those components in their output.

Comment 25

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8672059 [details]
MozReview Request: [mq]: dummy2

Bug 1203266 - Optionally read manifestparser manifests with a Finder class. r=ahal

We need to go through an abstraction layer when manifestparser manifests
are read as a part of reading moz.build files in case moz.build reading
is using mercurial as its filesystem. This adds an optional finder member,
which will be used if present for IO that happens when reading manifests,
This needs to be optional, because the manifestparser can be used as a
standalone package that isn't distributed with mozpack.

Comment 26

[Andrew Halberstadt [:ahal]]

Comment on attachment 8672059 [details]
MozReview Request: [mq]: dummy2

https://reviewboard.mozilla.org/r/21507/#review19705

Thanks!

Comment 27

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8669165 [details]
MozReview Request: [mq]: dummy1

Bug 1203266 - Don't call normpath in the manifestparser on paths that don't contain '..'. r=ahal

Profiling the manifestparser revealed that 25% of its total time is spent in a single
call to os.path.normpath, called on each test path in each manifest. For the manifests
in mozilla-central, these calls almost always return their input. To save time, this
patch modifies the manifestparser to not call normpath on paths that don't contain
'..'. This will change the parsed result of manifests containing no-op path
components ('//' or '/./') to include those components in their output.

Comment 28

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8672059 [details]
MozReview Request: [mq]: dummy2

Bug 1203266 - Optionally read manifestparser manifests with a Finder class. r=ahal

We need to go through an abstraction layer when manifestparser manifests
are read as a part of reading moz.build files in case moz.build reading
is using mercurial as its filesystem. This adds an optional finder member,
which will be used if present for IO that happens when reading manifests,
This needs to be optional, because the manifestparser can be used as a
standalone package that isn't distributed with mozpack.

Comment 29

[Chris Manchester (limited bugmail, email directly)]

Attachment: MozReview Request: [mq]: dummy3

Bug 1203266 - Allow reading reftest manifests from a provided finder. r=gps

Comment 30

[Chris Manchester (limited bugmail, email directly)]

Attachment: MozReview Request: Bug 1203266 - Add a list variant to mozbuild.util that allows pre-processing list items with a provided callable. r=gps

Bug 1203266 - Add a list variant to mozbuild.util that allows pre-processing list items with a provided callable. r=gps

Comment 31

[Chris Manchester (limited bugmail, email directly)]

Attachment: MozReview Request: Bug 1203266 - Read test manifests in the reader instead of the emitter to make them available earlier in the build. r=gps

Bug 1203266 - Read test manifests in the reader instead of the emitter to make them available earlier in the build. r=gps

Comment 32

[Chris Manchester (limited bugmail, email directly)]

Attachment: MozReview Request: Bug 1203266 - Remove uses of the emitter in files_info now that they're no longer needed. r=gps

Bug 1203266 - Remove uses of the emitter in files_info now that they're no longer needed. r=gps

Comment 33

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8674585 [details]
MozReview Request: [mq]: dummy3

This was already reviewed. Pushing part of the series earlier skewed things.

Comment 34

[Gregory Szorc [:gps]]

Comment on attachment 8674585 [details]
MozReview Request: [mq]: dummy3

https://reviewboard.mozilla.org/r/22259/#review19921

Comment 35

[Gregory Szorc [:gps]]

Comment on attachment 8674586 [details]
MozReview Request: Bug 1203266 - Add a list variant to mozbuild.util that allows pre-processing list items with a provided callable. r=gps

https://reviewboard.mozilla.org/r/22261/#review19923

::: python/mozbuild/mozbuild/util.py:267
(Diff revision 1)
> +        if iterable is None:
> +            iterable = []

This /could/ be written as `iterable = iterable or []`. But checking against the explicit default argument value is OK (and arguably more correct).

::: python/mozbuild/mozbuild/util.py:395
(Diff revision 1)
> +        if action is None:
> +            raise ValueError('A callabe action is required to construct '
> +                             'a ListWithAction')

if not callable(action):

::: python/mozbuild/mozbuild/util.py:429
(Diff revision 1)
> +    A callable (action) may optionally be passed to the constructor to run on

You say this is optional but LisWithActionMixin() requires action isn't None.

Comment 36

[Gregory Szorc [:gps]]

Comment on attachment 8674587 [details]
MozReview Request: Bug 1203266 - Read test manifests in the reader instead of the emitter to make them available earlier in the build. r=gps

https://reviewboard.mozilla.org/r/22263/#review19941

This is good except for the injection of new dependencies into the reader. You sadly need to bloat scope to fix the packaging situation for these manifest parsers :/

::: python/mozbuild/mozbuild/frontend/context.py:558
(Diff revision 1)
> +    class _TypedListWithAction(ContextDerivedValue, TypedList(typ), ListWithAction):
> +        def __init__(self, context, *args):
> +            def _action(item):
> +                return item, action(context, item)
> +            super(_TypedListWithAction, self).__init__(action=_action, *args)

Your Python skills are strong.

::: python/mozbuild/mozbuild/testing.py:18
(Diff revision 1)
> +import reftest

This is going to introduce a new dependency that AFAIK isn't officially packaged. This will require a little extra work for hg.mozilla.org.

We should have a followup to get the reftest manifest parser integrated into the more formal Python packages.

::: python/mozbuild/mozbuild/testing.py:312
(Diff revision 1)
> +        execfile(paths_file, _globals)

This will break in Mercurial reading mode because it is relying on the filesystem to read content.

::: python/mozbuild/mozbuild/testing.py:313
(Diff revision 1)
> +        import manifest as wptmanifest

Same issue as reftest Python modules.

Comment 37

[Gregory Szorc [:gps]]

Comment on attachment 8674588 [details]
MozReview Request: Bug 1203266 - Remove uses of the emitter in files_info now that they're no longer needed. r=gps

https://reviewboard.mozilla.org/r/22265/#review19943

Comment 38

[Chris Manchester (limited bugmail, email directly)]

I'll land the first parts now until we figure out packaging.

Comment 39

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bd3b3a2b32a2
https://hg.mozilla.org/integration/mozilla-inbound/rev/c5e7d0672727
https://hg.mozilla.org/integration/mozilla-inbound/rev/98e03b1c4713

Comment 40

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/bd3b3a2b32a2
https://hg.mozilla.org/mozilla-central/rev/c5e7d0672727
https://hg.mozilla.org/mozilla-central/rev/98e03b1c4713

Comment 41

[Chris Manchester (limited bugmail, email directly)]

https://reviewboard.mozilla.org/r/22263/#review19941

> This will break in Mercurial reading mode because it is relying on the filesystem to read content.

As discussed on irc and in bug 1252798, I think we already expose this hazard (because calling files_info will run the emitter in order to read manifests).

Comment 42

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8669165 [details]
MozReview Request: [mq]: dummy1

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/21135/diff/4-5/

Comment 43

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8672059 [details]
MozReview Request: [mq]: dummy2

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/21507/diff/3-4/

Comment 44

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8674585 [details]
MozReview Request: [mq]: dummy3

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/22259/diff/1-2/

Comment 45

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8674586 [details]
MozReview Request: Bug 1203266 - Add a list variant to mozbuild.util that allows pre-processing list items with a provided callable. r=gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/22261/diff/1-2/

Comment 46

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8674587 [details]
MozReview Request: Bug 1203266 - Read test manifests in the reader instead of the emitter to make them available earlier in the build. r=gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/22263/diff/1-2/

Comment 47

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8674588 [details]
MozReview Request: Bug 1203266 - Remove uses of the emitter in files_info now that they're no longer needed. r=gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/22265/diff/1-2/

Comment 48

[Gregory Szorc [:gps]]

Comment on attachment 8674587 [details]
MozReview Request: Bug 1203266 - Read test manifests in the reader instead of the emitter to make them available earlier in the build. r=gps

https://reviewboard.mozilla.org/r/22263/#review34531

::: python/mozbuild/mozbuild/frontend/context.py:603
(Diff revision 2)
> +    invokes the given given callable with each input and a context as it is

Nit: "given given"

::: python/mozbuild/mozbuild/frontend/context.py:619
(Diff revision 2)
> +ManifestparserManifestList = OrderedListWithAction(read_manifestparser_manifest)
> +ReftestManifestList = OrderedListWithAction(read_reftest_manifest)
> +WptManifestList = TypedListWithAction(WebPlatformTestManifest, read_wpt_manifest)

One thing that could bite us with these WithAction types is that the item assigned to the list is no longer what you think. As long as we're not hanging flags off items or searching for items from moz.build, I think we'll be fine.

::: python/mozbuild/mozbuild/testing.py:311
(Diff revision 2)
> +    old_path = sys.path[:]
> +    try:
> +        # Setup sys.path to include all the dependencies required to import
> +        # the web-platform-tests manifest parser. web-platform-tests provides
> +        # a the localpaths.py to do the path manipulation, which we load,
> +        # providing the __file__ variable so it can resolve the relative
> +        # paths correctly.
> +        paths_file = os.path.join(context.config.topsrcdir, "testing",
> +                                  "web-platform", "tests", "tools", "localpaths.py")
> +        _globals = {"__file__": paths_file}
> +        execfile(paths_file, _globals)
> +        import manifest as wptmanifest
> +    finally:
> +        sys.path = old_path

Congratulations on becoming the blame owner for this code!

Comment 49

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/98c44d678dc3
https://hg.mozilla.org/integration/mozilla-inbound/rev/fd993badf8b7
https://hg.mozilla.org/integration/mozilla-inbound/rev/bb0d852716ac

Comment 51

[Andrew Halberstadt [:ahal]]

\o/

Does this make bugs like bug 1209248 obsolete? Or will there be some follow up work for things like |mach try| to reap the benefits?

Comment 52

[Andrew Halberstadt [:ahal]]

Sorry that's not the bug I was thinking of. I'm asking if |mach try| will work without a build now when you specify test paths.

Comment 53

[Chris Manchester (limited bugmail, email directly)]

(In reply to Andrew Halberstadt [:ahal] from comment #52)
> Sorry that's not the bug I was thinking of. I'm asking if |mach try| will
> work without a build now when you specify test paths.

Not quite, because the test resolver uses all-tests.json, which is written by the build backend, but having a resolver that works from file system reading mode is much easier to imagine now.

Comment 54

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/98c44d678dc3
https://hg.mozilla.org/mozilla-central/rev/fd993badf8b7
https://hg.mozilla.org/mozilla-central/rev/bb0d852716ac

Comment 55

[BugBot [:suhaib / :marco/ :calixte]]

The leave-open keyword is there and there is no activity for 6 months.
:kmoir, maybe it's time to close this bug?

Comment 56

[Kim Moir [:kmoir] ET]

Chris, it seems like you worked extensively on this bug.  Can it be closed or should it be left open as per the leave-open keyword?  Is there still work left to do or is this an inactive bug?

Comment 57

[Chris Manchester (limited bugmail, email directly)]

I'm pretty sure everything relevant here landed.