Replace ev and eio calls with uv

[evanp]

So, this pull request replaces the old EIO and EV calls with calls to libuv instead. Those are preferred for 0.8.x and now mandatory for 0.10.x.

Right now the request builds but I still get errors in tests; I probably need to handle passing the results and errno back and forth in a different way, and I'm not sure about the way the uv_ref and uv_unref calls are made.

But I still wanted to get it started to get the discussion going.

[baudehlo]

OK so I'll hold off on merging this until you have it working.

[evanp]

It's my first time working with this codebase; can you verify that there's an error?

[ghost]

I accidentally just duplicated efforts here -- but I only get one test failure, and that same test fails when I use the current version from the repo (without the patch). It's over here -- https://github.com/baudehlo/node-fs-ext/pull/18

(Edit: I think both of our patches are essentially identical. The test failure that I'm seeing, with or without the patch, is below.)

FAILURE: expect_errno: seekSync(): expected errno 'EINVAL', but got result 17592186044416
Tests run: 31     ok: 30

[evanp]

@browndav There are some small differences. Can we reconcile them?

[ghost]

Sure thing. I'll test your patch really quickly and see if there's any functional difference.

[ghost]

I just tried your version -- it looks like there's an unused int status argument on the After function, and the compiler can't seem to resolve error_t as a type. Otherwise I think it's identical (other than the errorno vs. error member item naming). I seem to recall that there is a proper error type that libuv exposes, but I'm blanking on it at the moment.

[ghost]

I think if you change error_t to int and the prototype of After(uv_work_t *, int) to After(uv_work_t *) yours will work without any issues. Still seeing that same test failure, but I don't think it's at all related to your patch.

[ghost]

I'm on Mac OS X FWIW, re: the test failure. I'll try on a Linux box and see if it happens there too.

[baudehlo]

OK so which pull request should I merge? ;-)

On Mon, Mar 18, 2013 at 4:41 PM, David Brown notifications@github.comwrote:

I'm on Mac OS X FWIW, re: the test failure. I'll try on a Linux box and see if it happens there too.

— Reply to this email directly or view it on GitHubhttps://github.com/baudehlo/node-fs-ext/pull/16#issuecomment-15080368 .

[ghost]

Merge both and resolve conflicts? ducks

As far as I can tell, mine is exactly the same as evanp's, save for two minor syntax errors. Merge mine and credit him. :)

[evanp]

It sounds like @browndav 's is working; I'm not on the computer that has my 0.10.x setup so I can't verify right now.

So, please pull his and verify!

[baudehlo]

I've made you both contributors to the project. I don't have a whole lot of time for it right now, but can push to npm whenever you guys consider it ready to go. Merge away!

[ghost]

Merged pull request at https://github.com/baudehlo/node-fs-ext/pull/18.

I'd like to see about fixing the existing test failure before we push to npm. I'll try to test it on a couple other platforms and take a stab at a fix tonight.

Leaving this open for a bit just in case evanp has any issues.

[ghost]

I just tried the test suite on a Linux 3.8.2 machine and everything passed. I'm suspecting the seekSync test failure is OSX-specific.

On Linux:

$ ./run_tests 
Tests run: 31     ok: 31
Tests run: 26     ok: 26
Tests run: 11     ok: 11

On OS X:

$ ./run_tests 
FAILURE: expect_errno: seekSync(): expected errno 'EINVAL', but got result 17592186044416
Tests run: 31     ok: 30
One or more subtests failed!
Tests run: 26     ok: 26
Tests run: 11     ok: 11

[baudehlo]

So looking further into this test, I think it's just working now on OSX where it didn't before. It's a test for seeking beyond the 32bit limit IIRC. I've commented it out.

Any reason not to do an npm release now?

[ghost]

The only thing left (that I'm seeing) is a patch to get node-fs-ext to build for both Node v0.8.xx and v0.10.1 -- in EIO_After, Node v0.10.1 expects the second int argument, but Node v0.8.22 seems to want only the uv_work_t * argument. IIRC.

Maybe a bit of preprocessor magic to pick the right argument list? I can take a stab at it this evening if nobody else gets there first.

[ghost]

Update: I'm also seeing this on Node v10.0.1; checking in to it now.

Error: Symbol fs_ext_module not found.
    at Module.load (module.js:356:32)
    at Function.Module._load (module.js:312:12)
    at Module.require (module.js:364:17)
    at require (module.js:380:17)
    at Object.<anonymous> ($prefix/lib/node_modules/fs-ext/fs-ext.js:20:15)
    at Module._compile (module.js:456:26)
    at Object.Module._extensions..js (module.js:474:10)
    at Module.load (module.js:356:32)
    at Function.Module._load (module.js:312:12)
    at Module.require (module.js:364:17)

[ghost]

Easy enough; just need to add NODE_MODULE(fs_ext, init) at the bottom. Will send pull request.

[baudehlo]

It's a bit more complicated than that to work with both.

Fixed in commit 49bcf5c.