Node 0.11.x support #99

donpark · 2014-10-19T12:05:00Z

With these changes, node_mdns builds and runs under Node 0.11.14 and nan 1.3 although I cannot say with certainty as amount of testing it received with these changes is too little.

untested.

I only took a birdseye view at the code so excuse me if I'm mistaken, but shouldn't `errno` be `err`?

errno is undefined

donpark · 2014-10-19T12:07:43Z

This PR addresses issue #83

agnat · 2014-10-19T16:47:24Z

Thanks for stepping up, @donpark.

It is a pretty involved change set, though. I'll review it and comment in-line.

@ronkorving, could you do/organize a little gang testing?

donpark · 2014-10-19T17:56:45Z

It is a pretty involved change set, though. I'll review it and comment in-line.

Absolutely. I would've been rather surprised and dismayed if my 'carpet-bombing' PR got merged as-is.
Please do as you see fit. I also don't mind if you only use my PR as hints showing areas affected by Node 0.11.x and use your own changes. Frankly, I think later approach could require less work.

agnat · 2014-10-21T12:51:15Z

Uhu... I catch your drift. Thing is: Quite a while back I decided that I'm done with node. I'm done with it because of issues like this one. I'm fed up with the node teams attitude regarding native modules. So no, I won't fix another breakage caused by @joyent's inability or unwillingness to mitigate the consequences of their changes. I'm still trying to help people out but I won't do any active development. I'm sorry to do this but this is a spare time project and spare time should be fun.

However, I will review it and then we'll go from there. If you, @donpark, don't have the time to clean up the patch maybe someone else does.

agnat · 2014-10-21T13:07:50Z

lib/resolver_sequence_tasks.js

+          cb(undefined, []);
+        } else {
+          cb(errnoException(errno, 'getaddrinfo'));
+        }


Duplicate Code: The two versions of oncomplete are almost identical. The second version should call the first one and just inject the error argument.

donpark · 2014-10-21T14:07:53Z

NP. Felt the same frustration. I patched node_mdns for a prototype app using atomshell which saw the broadside of 0.11.x related nightmare. Yikes. But it's just a tool and this PR was requested by someone else. Let's just leave this as is for now. Anyone who needs it badly can use my fork.

agnat · 2014-10-21T14:15:38Z

src/txt_record_buffer_to_object.cpp

-        result->Set(String::New(&*key.begin()),
-                value_ptr ? 
-                String::New(static_cast<const char*>(value_ptr), value_length) :
-                Undefined());


My beautiful ternary operator! Gone. >:{

haha. ?: formatting fetish is bizarrely common.

agnat · 2014-10-21T20:20:03Z

A'ight.

BTW, @ronkorving: If you and the users decide to merge it as is without further testing or clean-ups: I'm good with that.

ronkorving · 2014-10-22T04:14:41Z

Hey guys, thanks for the work and review. I've been traveling so I have some catching up to do left and right. I'll give this PR a pass.

ronkorving · 2014-10-22T10:19:52Z

package.json

@@ -55,5 +55,10 @@
    , "github": "TobyEalden"
    }
  ]
+, "dependencies":
+  { "bindings": "*"
+  , "nan": "*"


I don't know how @agnat feels about this, but I'm very much against asterisking dependencies.

I'm very much against dependencies.

Both of them are tightly coupled to node. In fact they are part of node but... oh well. Pinning a version does not seem right either. We want the latest round of nan compatibility hacks, no? What do you suggest? >=?

At least ~. Changing a major version allows you to break all-the-things. We don't want to be hit by that at night when we're deploying our latest product, just because it coincided with a breaking release of a dependency (NAN) of a dependency (node-mdns).

You're much more experienced in actually running this kind of thing. So, it's up to you. As I said: I'm not ok with dependencies anyway. ;)

* was the reason the branch was named nan-latest. It should be fixed to a version after merge.

tilde (or that godawful caret) is good for NAN, we only do bug fixes in patch releases and promise not to break anything there. Minor versions are mostly safe as well, but stuff can get deprecated for good reasons. Major versions can do anything.

kkoopa · 2014-10-23T13:21:08Z

src/dns_service_browse.cpp

-    args[1] = Integer::New(flags);
-    args[2] = Integer::NewFromUnsigned(interfaceIndex);
-    args[3] = Integer::New(errorCode);
+    args[0] = NanNew(NanObjectWrapHandle(serviceRef));


Should be no need for the NanNew here, NanObjectWrapHandle returns a Local<Object>

donpark · 2014-10-23T15:10:36Z

But isn't it at least case preserving?

That would be my expectation too although all service types I've seen were lowercase.

Is that a linux box?

No, OS X Yosemite w/XCode 6.1.

NanObjectWrapHandle already returns a Local<Object>.

donpark · 2014-10-23T15:39:24Z

From page 5 of Bonjour Device Discovery.pdf:

(RFC 2782) Service Types: Short name of protocol, fourteen characters maximum, conforming to normal DNS host name rules: Only lower-case letters, digits, and hyphens; must begin and end with lower-case letter or digit.

Note this is not from RFC 2782 itself but just mentioned in DNS SRV-related slide created by two Apple engineer.

My theory: 1) Bonjour service type names are case insensitive and supports uppercase, but 2) DNS SRV record entry has to be lowercase, so 3) everyone used lowercase to avoid headaches.

donpark · 2014-10-23T15:41:56Z

Re simple browsing assert failures, they fail on node_mdns version 2.2.1 also so doesn't appear to be caused by this PR.

donpark · 2014-10-23T15:48:37Z

Found a more definite source. From Apple's MCNearbyServiceBrowser doc:

[serviceType] Must be 1–15 characters long. Can contain only ASCII lowercase letters, numbers, and hyphens.

Note there are some confusions over how many hyphens are allowed. What a fine mess.

see https://developer.apple.com/Library/ios/documentation/MultipeerConnectivity/Reference/MCNearbyServiceBrowserClassRef/index.html#//apple_ref/occ/instm/MCNearbyServiceBrowser/initWithPeer:serviceType:

agnat · 2014-10-23T16:21:41Z

Nice. You already found and fixed the char set. I guess newer versions just enforce lowercase service types. That would explain the test failures.

All tests pass on OS X 10.9 node v0.10.32.

Let's give other interested parties a couple of hours to run the tests and we're good to go.

Node 0.11.x support

agnat · 2014-10-25T14:54:57Z

Thanks everybody for this great PR: @kkoopa, @ronkorving, @y-lohse and most of all @donpark who did the work and pushed mdns to the future.

Great job.

y-lohse · 2014-10-27T08:56:41Z

I clearly did the biggest part.

Joke aside, I tested on windows this morning, works fine. The tests fail at some point but I never ran them before so it might not be related to this PR. Here's the log:

Browser
? Should retrieve interface name from cache when interface index is no longer va
lid  [4/4]

test_dns_sd
? DNSServiceRef                 [3/3]
? DNSServiceRegister()          [20/20]
? DNSServiceProcessResult()     [13/13]
? DNSServiceRefSockFD()         [6/6]
? DNSServiceBrowse()            [13/13]
? DNSServiceRefDeallocate()     [8/8]
? DNSServiceResolve()           [11/11]
? DNSServiceEnumerateDomains()  [13/13]
? DNSServiceGetAddrInfo         [1/1]
? TXTRecordRef                  [31/31]
? buildException()              [47/47]
? exportConstants()             [3/3]

test_functional
? simple browsing                                       [116/116]
? create ads                                            [8/8]
[SKIPPED] write better test for browseThemAll()
? browseThemAll()                                       [0/0]
? resolver sequence                                     [16/16]
C:\Users\caldera\Documents\jdf\node_modules\mdns\lib\network_interface.js:36
      index = dns_sd.if_nametoindex(networkInterface);
                     ^
Error: failed to convert interface alias to luid
    at Error (native)
    at Object.interfaceIndex (C:\Users\caldera\Documents\jdf\node_modules\mdns\l
ib\network_interface.js:36:22)
    at new Browser (C:\Users\caldera\Documents\jdf\node_modules\mdns\lib\browser
.js:15:23)
    at Object.create [as createBrowser] (C:\Users\caldera\Documents\jdf\node_mod
ules\mdns\lib\browser.js:104:10)
    at exports.local advertisement invisible on external interfaces (C:\Users\ca
ldera\Documents\jdf\node_modules\mdns\tests\test_functional.js:377:30)

Otherwise, thanks for the great job :)

ronkorving · 2014-10-27T09:16:38Z

Would you mind taking a closer look into that? Not to sound too stupid I hope, but what's an luid anyway?

agnat · 2014-10-27T09:23:51Z

ludicrous identifier... or something.

ronkorving · 2014-10-27T09:29:14Z

I knew it was something super duper technical. I'm not cut out for this.

kkoopa · 2014-10-27T09:31:10Z

I was going to debug it, but don't have a Crapple ID or the necessary SDK, so screw that. Mac sucks.

agnat · 2014-10-27T09:32:41Z

IIRC, windows interface names are subject to i18n and admins can change them. The luid is an internal identifier to keep track of things.

Could be an actual bug or just another weak test, though...

agnat · 2014-10-27T09:46:15Z

I guess luid are locally unique. They are carefully crafted to collide across systems. Think id++... just much more
complicated.

y-lohse · 2014-10-27T10:46:13Z

I'm completely clueless regarding c++, I don't even know how to log an error… so i'm afraid I won't be able to help a lot.
In network_interface.js:36, networkInterface's value is Connexion au réseau local 3, which is the name of my network interface. Not sure how well the accent fares in there.

donpark · 2014-10-27T11:03:45Z

LUID is a 64-bit locally unique identifier returned by Windows IP Helper API function ConvertInterfaceAliasToLuid failure of which is mapped to "failed to convert interface alias to luid" error in network_interface.cpp.

This may have something to do with deprecation warning above error throwing call to dns_sd.if_nameToIndex:

 console.warn("WARNING: 'interfaceIndex' is deprecated and will be " +
      "removed. Please use 'networkInterface' instead and see the " +
      "documentation on why it's cool.");

agnat · 2014-10-27T11:34:03Z

Pretty sure the warning is unrelated. It's about an, uh, upcoming API change in node_mdns. Although the code tries to handle unicode names this never has been stress tested. So, blaming the accent seems like a good guess.

agnat · 2014-10-27T12:14:44Z

@y-lohse, could you paste the results of test_network_interface.js? Embarrassingly, the easiest way to do that seems to be renaming test_functional.js to dont_test_functional.js or some such. Then run npm test.

y-lohse · 2014-10-27T13:11:59Z

Sure thing:

`test_network_interface
C:\Users\caldera\Documents\jdf\node_modules\mdns\tests\test_network_interface.js
:25
      var index = dns_sd.if_nametoindex(name);
                         ^
Error: failed to convert interface alias to luid
    at Error (native)
    at exports.if_nametoindex <-> if_indextoname (C:\Users\caldera\Documents\jdf
\node_modules\mdns\tests\test_network_interface.js:25:26)
    at run_testcase (C:\Users\caldera\Documents\jdf\node_modules\mdns\utils\test
run:191:3)
    at Array.<anonymous> (C:\Users\caldera\Documents\jdf\node_modules\mdns\node_
modules\slide\lib\bind-actor.js:15:8)
    at LOOP (C:\Users\caldera\Documents\jdf\node_modules\mdns\node_modules\slide
\lib\chain.js:15:14)
    at chain (C:\Users\caldera\Documents\jdf\node_modules\mdns\node_modules\slid
e\lib\chain.js:20:5)
    at run_group (C:\Users\caldera\Documents\jdf\node_modules\mdns\utils\testrun
:171:5)
    at Array.<anonymous> (C:\Users\caldera\Documents\jdf\node_modules\mdns\node_
modules\slide\lib\bind-actor.js:15:8)
    at LOOP (C:\Users\caldera\Documents\jdf\node_modules\mdns\node_modules\slide
\lib\chain.js:15:14)
    at C:\Users\caldera\Documents\jdf\node_modules\mdns\node_modules\slide\lib\c
hain.js:18:7

Out of curiosity, I commented the failing test out and the rest of them pass.

agnat · 2014-10-27T18:13:54Z

Let's create a new issue for this one...

donpark · 2014-10-27T19:11:03Z

@agnat What I meant by related was the need for deprecation warning which points to changing environment, new code, and so on. Sorry, I tend to go meta at wrong times.

donpark and others added 8 commits June 24, 2014 16:29

compile using node 0.11.9

7832eec

untested.

node 0.11+ cares.getaddrinfo support

b359e50

Hack to fix "node.h missing ';' before identifier" build error

3de63b6

update SSIZE_T hack with version from latest node.h

c16a637

try version from node 0.11.13 with v8.h include

4879dad

fix compiler errors under nan 1.2

df6fe38

errno is undefined

2a58a9f

I only took a birdseye view at the code so excuse me if I'm mistaken, but shouldn't `errno` be `err`?

Merge pull request #1 from y-lohse/patch-1

ecdb423

errno is undefined

agnat reviewed Oct 21, 2014
View reviewed changes

ronkorving reviewed Oct 22, 2014
View reviewed changes

kkoopa reviewed Oct 23, 2014
View reviewed changes

no need for the NanNew.

e1796e4

NanObjectWrapHandle already returns a Local<Object>.

donpark added 2 commits October 23, 2014 08:51

service type must be lowercase, numbers, or hyphens.

3285be0

see https://developer.apple.com/Library/ios/documentation/MultipeerConnectivity/Reference/MCNearbyServiceBrowserClassRef/index.html#//apple_ref/occ/instm/MCNearbyServiceBrowser/initWithPeer:serviceType:

restore numbers removed mistakenly.

b6dd95b

pin-down dependency versions

3c65600

agnat added a commit that referenced this pull request Oct 25, 2014

Merge pull request #99 from donpark/nan-latest

0bd4053

Node 0.11.x support

agnat merged commit 0bd4053 into agnat:master Oct 25, 2014

agnat mentioned this pull request Oct 27, 2014

if_nametoindex(...) fails on windows #100

Closed

This was referenced Oct 28, 2014

NAN 2 Discussion nodejs/nan#208

Closed

Requiring module from Node-webkit : "Module did not self-register" #101

Closed

Node 0.11.x support #99

Node 0.11.x support #99

Conversation

donpark commented Oct 19, 2014

donpark commented Oct 19, 2014

agnat commented Oct 19, 2014

donpark commented Oct 19, 2014

agnat commented Oct 21, 2014

Choose a reason for hiding this comment

donpark commented Oct 21, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agnat commented Oct 21, 2014

ronkorving commented Oct 22, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

donpark commented Oct 23, 2014

donpark commented Oct 23, 2014

donpark commented Oct 23, 2014

donpark commented Oct 23, 2014

agnat commented Oct 23, 2014

agnat commented Oct 25, 2014

y-lohse commented Oct 27, 2014

ronkorving commented Oct 27, 2014

agnat commented Oct 27, 2014

ronkorving commented Oct 27, 2014

kkoopa commented Oct 27, 2014

agnat commented Oct 27, 2014

agnat commented Oct 27, 2014

y-lohse commented Oct 27, 2014

donpark commented Oct 27, 2014

agnat commented Oct 27, 2014

agnat commented Oct 27, 2014

y-lohse commented Oct 27, 2014

agnat commented Oct 27, 2014

donpark commented Oct 27, 2014