Module path search order is undefined/undocumented

RESOLVED FIXED in 1.0

Status

defect
P2
normal
RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: avarma, Assigned: warner)

Tracking

unspecified

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

Reporter

Description

9 years ago
When cfx runs a Jetpack program, the order of paths searched through during a require() call is undocumented and/or undefined.
Atul suggests that this would be important to fix in 0.3.  Brian: does this seem like something you would be able to take on?
OS: Mac OS X → All
Hardware: x86 → All
Target Milestone: -- → 0.3
yeah, I'll take this one.

My inclination is to mostly stick with the Python approach. But from what I
can tell, cfx's equivalent to python's "sys.path" is basically defined by:

 [for pkgname in os.list(SDKROOT+"/packages")]

Of the four packages we have right now (development-mode, jetpack-core,
nsjetpack, and test-harness), I think we want (jetpack-core, nsjetpack) to be
first, and (development-mode, test-harness) to maybe not even be searched
except during the execution of "cfx test". I expect we'll be adding a package
for user-generated or user-downloaded APIs (the "cfx install" tool that I
heard Atul talk about in the last meeting), and of course the actual
user-developed add-on code will live somewhere.

I think a plausible good search path is thus (from highest priority to
lowest):

 user-developed add-on pkgdir
 "cfx install" -supplied packages
 nsjetpack
 jetpack-core
 development-mode
 test-harness

However, I think we either need to establish some distinctive/magic names for
the package dirs (and thus have some weird rules for handling unrecognized
dirs), or establish a strict alphanumeric-sort rule (and rename the existing
directories to achieve the ordering we want).

The former would be accomplished by something like:

 dirs = sorted(os.listdir(SDKROOT+"/packages"))
 dirs.remove("nsjetpack"); dirs.append("nsjetpack")
 dirs.remove("jetpack-core"); dirs.append("jetpack-core")
 dirs.remove("development-mode"); dirs.append("development-mode")
 dirs.remove("test-harness"); dirs.append("test-harness")

The latter would be accomplished with:

 dirs = sorted(os.listdir(SDKROOT+"/packages"))
 # packages/10_userstuff
 # packages/80_installed_packages
 # packages/90_nsjetpack
 # packages/92_jetpack-core
 # packages/95_development-mode
 # packages/96_test-harness


I guess I'm leaning towards the former right now, even though the rules are a
bit weirder, because in the latter approach a user who creates "packages/foo"
will be disappointed to discover (after much confusion) that their stuff is
found last, not first.
Assignee

Updated

9 years ago
Assignee: nobody → warner-bugzilla
Would it help to subdivide the packages/ directory into system/ and user/ subdirectories (or the like), with explicit rules for which get loaded first (and strict alphanumeric loading within each directory)?  That way we avoid magic package directory reordering while still achieving the goal of loading user packages first.
Reporter

Comment 4

9 years ago
Uh, weird. I had actually assumed that package search order would be defined by the order in which the parent package's "dependencies" key listed them, so that e.g. if package "foo" had a package.json that looked like this:

  {
    "name": "foo",
    "dependencies": ["bar", "baz"]
  }

Then module-name collisions between "bar" and "baz" would be resolved by "bar" taking higher priority. Alternatively, if the order of "dependencies" were reversed, then "baz" would take priority. And if "foo" itself had a module with the same name as something in bar and/or baz, then it would take even higher priority.

The thing I like about this approach is that it leaves the creator of the package/addon/app/whatever in full control of their effective sys.path, since "dependencies" maps directly to a directory search order. The thing I'm concerned about with it, though, is that this needs to be explicitly understood by developers: if someone doesn't know that setting "dependencies" to ["bar", "baz"] could result in completely different behavior than ["baz", "bar"], they may end up feeling confused, alienated, and alone.
Reporter

Comment 5

9 years ago
Also note that this potentially has implications for bug 560716 comment 4, if we decide to pursue a Narwhal-esque approach to app/platform-specific behavior.
Reporter

Comment 6

9 years ago
Arg, so this apparently isn't working as I hoped it would be: the addons-builder-helper addon (formerly called the FlightDeck addon) cites the 'development-mode' package as a dependency in its package.json, yet that package's main.js is overriding the ABH's (causing me to rename ABH's main.js to something else and pointing at it from the ABH's package.json as a temporary workaround).

I need to fix this, shouldn't be too hard.
Reporter

Comment 7

9 years ago
Posted patch patchSplinter Review
Reporter

Comment 8

9 years ago
Here's one tentative fix; the module search order is apparently the exact reverse of what it should be.
Reporter

Updated

9 years ago
Attachment #449792 - Flags: review?(warner-bugzilla)
Comment on attachment 449792 [details] [diff] [review]
patch

Behavior looks good to me, I now see jetpack-core-lib showing up at the end
of my harness-options.json "rootPaths" instead of the beginning, which I
assume means that the runtime's require() will find modules there first.

I think we need some docs somewhere, because I'm not finding anything in 'cfx
docs' that talks about search order. I was previously confused about how it
all works (thinking that there was an equivalent to $PATH, $PYTHONPATH, or
$LDPATH), and I suspect others will be similarly confused in the future. I'll
throw down some notes here in case they are useful to anyone who writes some
docs.

In those other $PATH scenarios, you have multiple directories and a bunch of
executables in each one, and any searching is done over a list of
directories. In contrast, Jetpack has three things: directories, packages,
and modules, and searching is done over a list of packages.

The "top-level package" is defined as whatever the CWD is when you run 'cfx
xpi', and this package is always first on the search path.

The only other place where packages live is in the $SDKTOP/packages directory
(where you find things like jetpack-core). It contains multiple packages, one
per subdirectory. By default, the package name is the same as the
subdirectory name, providing a natural collision-resistance, but packages can
override their name by providing one in their package.json, so it is possible
to get colliding package names even between two subdirs of $SDKTOP/packages .

Package names are not allowed to collide: the SDK will throw an error if it
sees two package of the same name.

The package search path is controlled by "dependencies" lines in each
package's package.json file. In particular, the SDK starts by adding the
top-level package to the search path. Then it walks through
toplevel["dependencies"] entries, treating each as a package name, looking in
$SDKTOP/packages for a matching package, and appending the result to the
search path. This traversal is performed depth-first. Finally, when
everything else has been added, the "jetpack-core" package is appended.
Attachment #449792 - Flags: review?(warner-bugzilla) → review+
Assignee

Updated

9 years ago
Assignee: warner-bugzilla → avarma
The Add-on SDK is no longer a Mozilla Labs experiment and has become a big enough project to warrant its own Bugzilla product, so the "Add-on SDK" product has been created for it, and I am moving its bugs to that product.

To filter bugmail related to this change, filter on the word "looptid".
Component: Jetpack SDK → General
Product: Mozilla Labs → Add-on SDK
QA Contact: jetpack-sdk → general
Version: Trunk → unspecified
Brian: is this something that will be fixed by the other module loading work you're doing, and if not, can you take this on?
Assignee: avarma → warner-bugzilla
Priority: -- → P2
Target Milestone: 0.3 → 1.0
https://github.com/mozilla/addon-sdk/commit/658fb89ca4d3dc4a8cfb9616ae873e979a7f3955 lands a lot of linker cleanup, including a file (static-files/md/dev-guide/addon-development/module-search.md) which explains how the process works. It's not exactly like what we discussed here a last summer, so it'd be fair to open up a new ticket with any desired changes.
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.