jvilk / BrowserFS

BrowserFS is an in-browser filesystem that emulates the Node JS filesystem API and supports storing and retrieving files from various backends.
Other
3.07k stars 220 forks source link

Fix Arbitrary wildcard makefile build order (causes dev doppio builds to fail) #38

Closed angrave closed 11 years ago

angrave commented 11 years ago

Hi, I'm ran into some build issues with the latest version of doppio and browserfs.

Everything is fine if I used the pre-built git versions of vendor/browserfs/dist

browserfs also passes its tests.

However when I build my own browserfs and copy the new versions from browser/lib into browserfs/dist,

then I can no longer run doppio debug in the browser. but make doppio test passes! Grrrr. Here's the original symptom.

Line 699 BrowserFS.node.fs.Stats = function() { Uncaught TypeError: Cannot read property 'node' of undefined Stats.FILE = 1; Stats.DIRECTORY = 2; Stats.SYMLINK = 3; Stats.SOCKET = 4;

The underlying cause: a build order / dependency issue because Make 'wildcard' does not gauarnantee a build order. Using Make's sort function is a reaonabale workaround, if you're happy requiring a modern version of Make .

FYI steps to finding the problem.

In the official 'dist' builds Browser.node is defined early ...

grep 'BrowserFS.node.*=' dist-orig/browserfs.js

        BrowserFS.node = {};
    BrowserFS.node.Buffer = function() {
    BrowserFS.node.fs = function() {
    BrowserFS.node.fs.Stats = function() {
    BrowserFS.node.path = function() {
                    resolved = BrowserFS.node.path.normalize(cwd + (cwd !== "/" ? path.sep : "") + resolved);
    BrowserFS.node.process = function() {
                data = BrowserFS.node.Buffer((_ref2 = req.response) != null ? _ref2 : 0);

In my locally built version, BrowserFS.node = {} appears too late...

grep 'BrowserFS.node.*=' dist/browserfs.js

    BrowserFS.node.fs.Stats = function() { <-- fs_stats TOO EARLY!
    BrowserFS.node.process = function() {
    BrowserFS.node.path = function() {
                    resolved = BrowserFS.node.path.normalize(cwd + (cwd !== "/" ? path.sep : "") + resolved);
    BrowserFS.node.Buffer = function() {
    BrowserFS.node.fs = function() { <-- SHOULD BE BEFORE fs.Stats above
        BrowserFS.node = {};  <-- 000-browserfs.js SHOULD BE FIRST
                data = BrowserFS.node.Buffer((_ref2 = req.response) != null ? _ref2 : 0);

My suspicion fell on the following ... SRCS_CORE := $(wildcard src/core/*.coffee)

FYI On my OSX machine with Make v3.82 SRCS_CORE := $(wildcard src/core/*.coffee) is not sorted In fact, a google search shows this is true for 3-.82 onwards, despite what the docs say -

https://bugzilla.redhat.com/show_bug.cgi?id=635607 "The NEWS file for make-3.82 says:

If I'm reading "up to and including this release" right, the order was NOT supposed to change in this version. But it did.

"

Fortunately, Make supports a sort command, so I'll send a pull-request with the following -

From Make 3.82 onwards, wildcard returns filenames in arbitrary order.

#Alphabetically sorting the values is sufficient for browserfs build dependencies.
SRCS_CORE_UNSORTED := $(wildcard src/core/*.coffee)
SRCS_CORE := $(sort $(SRCS_CORE_UNSORTED))

Alternatively one could split the core, or just explicitly name the files in the Makefile. Best, Lawrence.

perimosocordiae commented 11 years ago

Thanks! Could we just do:

SRCS_CORE := $(sort $(wildcard src/core/*.coffee))

Alternatively, @jvilk: if the core files are stable, it would be easy enough to list them explicitly.

jvilk commented 11 years ago

Why is listing them explicitly preferred to sorting them? Sorting is much lower maintenance. There should be limited dependencies among files within the non-core directory.

Also, thanks for all of these fixes, @angrave !