1065434 - unify path handling

Status: NEW

(Firefox Build System :: General, defect)

Description

[Nathan Froyd [:froydnj]]

bug 976733 comment 37:

glandium wrote some very similar code in bug 1062221. I gave r+ to his patches. But I don't want to delay this awesome patch from landing. Please file a follow-up to unify the path handling code.

Comment 1

[Nathan Froyd [:froydnj]]

So I've been looking at this, and have a series of 6 (!) patches that addresses almost all of this.  Except that I'm running into a small problem.  TypedList contains this innocuous-looking comment:

http://dxr.mozilla.org/mozilla-central/source/python/mozbuild/mozbuild/util.py#870

    '''A list with type coercion.

    The given ``type`` is what list elements are being coerced to. It may do
    strict validation, throwing ValueError exceptions.

    A ``base_class`` type can be given for more specific uses than a List. For
    example, a Typed StrictOrderingOnAppendList can be created with:

       TypedList(unicode, StrictOrderingOnAppendList)
    '''

Which sounds great in theory, but consider something like:

  SourcePathList = TypedList(SourcePath, StrictOrderingOnAppendList)

to define a list of source paths, which can automagically resolve paths appended to the list from the current Context.  Then consider something like:

http://dxr.mozilla.org/mozilla-central/source/dom/media/gtest/moz.build#24

TEST_HARNESS_FILES.gtest += [
    '../test/gizmo.mp4',
    'mediasource_test.mp4',
    'test.webm',
]

where TEST_HARNESS_FILES uses the above SourcePathList to store "strings" in its hierarchy of nodes.

You now have a problem.  The incoming list in moz.build *is* sorted correctly.  But because TypedList (really, TypedListMixin) applies type coercion prior to calling its superclass's methods, what StrictOrderingOnAppendList actually sees from the above is more like:

  [SourcePath('../test/gizmo.mp4'), SourcePath('mediasource_test.mp4'), ...]

And because we eagerly coerce the SourcePaths to strings due to this line in StrictOrderingOnAppendList:

  srtd = sorted(l, key=lambda x: x.lower())

(so you can't simply define __cmp__ methods on SourcePath to operate on unresolved paths, instead of resolved ones) we wind up with something like this:

  ['$TOPSRCDIR/dom/media/test/gizmo.mp4', '$TOPSRCDIR/dom/media/gtest/mediasource_test.mp4', ...]

which is *not* sorted correctly, and moz.build complains.

Better method combination mechanisms would fix this, but we are stuck with Python.  I'm unsure of how to fix this without hacking in knowledge of StrictOrderingOnAppendList directly to TypedListMixin.  Any better ideas?

Comment 2

[Nathan Froyd [:froydnj]]

Attachment: part 1 - move memoize and memoized_property higher up in the file

[attaching these so people can see and comment if they like]

Unlike function calls in Python, where the name doesn't have to come
prior to the call, decorators require that the name has already been
seen when the decorator is used.  We'll use @memoize in subsequent
part(s), so ensure that memoize's definition is visible throughout the
file.

Comment 3

[Nathan Froyd [:froydnj]]

Attachment: part 2 - generalize HierarchicalStringList* to work with arbitrary types

Path information in moz.build files is represented as strings, but some
variables (e.g. TEST_DIRS) translate these strings to a richer internal
format for ease of handling.  Such variables are represented by a
ContextDerivedTypedList(SourcePath).  To extend this richer handling to
hierarchical objects (such as TEST_HARNESS_FILES), we need to generalize
HierarchicalStringList similarly, so that its members can be whatever
type we like.

This patch refactors the classes and functions to provide for general
HierarchicalTypedLists.

Comment 4

[Nathan Froyd [:froydnj]]

Attachment: part 3 - extend SourcePath to handle !-prefixed paths

The logic for resolving strings to paths is currently split into two
separate places: ContextDerived.resolve_path and SourcePath.data.  This
change fixes up the latter to honor the same rules as the former.

Comment 5

[Nathan Froyd [:froydnj]]

Attachment: part 4 - add is_{relative_,top}objdir_path methods to SourcePath

ContextDervied has a @staticmethod is_objdir_path that is required in a
few places; replacing those uses requires replicating the necessary
functionality in SourcePath.  Consolidating all the sigils-for-path
logic in SourcePath is a good idea in any event.

Comment 6

[Nathan Froyd [:froydnj]]

Attachment: part 5 - add ContextDerivedHierarchicalTypedList

The end state of this series is being able to define something like
TEST_HARNESS_FILES as a hierarchical list of SourcePath values.  Since
SourcePath is a ContextDerivedValue, we need a container that will
ensure that its contained SourcePath values are constructed with the
current Context.  Hence, we need a container that derives from
ContextDerivedValue (to get the Context from moz.build) and builds
appropriate types for HierarchicalTypedList, so HierarchicalTypedList's
elements (SourcePath in this case) can be constructed correctly.

Comment 7

[(not currently active) Ted Mielczarek]

Um, I think the problem is actually here:
http://dxr.mozilla.org/mozilla-central/source/python/mozbuild/mozbuild/frontend/context.py#296

If you're just blindly instantiating SourcePath objects, you actually get back unicode objects. The other stuff in context.py uses ContextDerivedTypedList to properly instantiate things:
http://dxr.mozilla.org/mozilla-central/source/python/mozbuild/mozbuild/frontend/context.py#354

I tested this and sorting real SourcePaths seems fine:
>>> b = MozbuildObject.from_environment()
>>> e = b.config_environment
>>> c = Context(config=e)
>>> c.push_source('/build/mozilla-central/moz.build')
>>> l = [SourcePath(c, 'a/b'), SourcePath(c, 'c/d')]
>>> sl = sorted(l, key=lambda x: x.lower())
>>> type(sl[0])
<class 'mozbuild.frontend.context.SourcePath'>

Comment 8

[(not currently active) Ted Mielczarek]

We conferred on IRC and I realized my error. The x.lower() bit in the sorted call is where things are going south, because the *sort keys* are coerced to unicode and thus can no longer sort intelligently.

Comment 9

[Nathan Froyd [:froydnj]]

Ted suggested on IRC we define some sort of 'source_representation' hook that StrictOrderingOnAppendList can call in preference to .lower(), if it exists.  This would solve the immediate problem and also ensure that we could report reasonable error messages to the user (e.g. the paths we report for unsorted things could be exactly what was written in the moz.build file instead of something with $topsrcdir prepended to it).

Comment 10

[Mike Hommey [:glandium]]

If you want my opinion, I think we should just get rid of those recursive classes. I already didn't like them when they were added, and their bastardization to support hyphens made them even worse.

Comment 11

[Nathan Froyd [:froydnj]]

(In reply to Mike Hommey [:glandium] from comment #10)
> If you want my opinion, I think we should just get rid of those recursive
> classes. I already didn't like them when they were added, and their
> bastardization to support hyphens made them even worse.

Having thought through this a little bit more...you actually want to get rid of HierarchicalTypedList, is that right?  What do you see it being replaced with?

Comment 12

[Nathan Froyd [:froydnj]]

We could almost do something like:

https://www.python.org/dev/peps/pep-0428/#joining

to wit:

EXTRA_JS_MODULES / path / in / tree += [ ... ]

Except that now we have barewords (|path|, |in|, etc.) that would be parsed in the wrong way, and requiring strings there would be somewhat inconvenient.  Or perhaps:

EXTRA_JS_MODULES.location('path', 'in', 'tree') += [ ... ]

Either way, finding some other syntax for this path-in-mozbuild-objects would open up cleaning up HierarchicalList.

Comment 13

[Mike Hommey [:glandium]]

Why not simply EXTRA_JS_MODULES['path/in/tree']?