Open Bug 1065434 Opened 10 years ago Updated 2 years ago

unify path handling

Categories

(Firefox Build System :: General, defect)

defect

Tracking

(Not tracked)

People

(Reporter: froydnj, Unassigned)

References

Details

Attachments

(5 files)

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.
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?
Flags: needinfo?(mh+mozilla)
[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.
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.
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.
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.
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.
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'>
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.
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).
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.
Flags: needinfo?(mh+mozilla)
(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?
Flags: needinfo?(mh+mozilla)
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.
Why not simply EXTRA_JS_MODULES['path/in/tree']?
Flags: needinfo?(mh+mozilla)
Product: Core → Firefox Build System
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: