Orion98MC / node_rrd

A node.js native binding for RRDtool (node rrd)
97 stars 25 forks source link

For build issues node_rdd:11,12,13 #15

Closed tdjordan closed 8 years ago

tdjordan commented 8 years ago

This gives a clean compile for build issues node_rdd:11,12,13 under node 4.1.1 using gcc 4.9.2 and/or clang 3.6.0 on Ubuntu 15.04

tdjordan commented 8 years ago

Reference Issue: https://github.com/Orion98MC/node_rrd/issues/11

Orion98MC commented 8 years ago

Great, thanks @tdjordan !

I am a bit ashamed that Nan reworded the NAN_METHOD "args" into "info" but I assume they did it for a reason. If anyone cares to enlighten me I'd be pleased.

While reviewing the code I kept thinking it should be good at some point to get rid of the old "#define"s used for arguments checking. There seems to be a better way of doing it with NanCheck().

Now, my other concern is whether I should merge this in master or in a separate branch. I'm open to discussion here.

tdjordan commented 8 years ago

Hi Thierry,

Before you merge, I'm going to see if I can get a clean compile with node 0.12 with the same code base in the morning.

I think I know at least one of the issues with 0.12. And I think there are two ways to solve it.

I should report back in late tomorrow afternoon (CDT timezone).

Yes, the args -> info change does seam arbitrary.

Tom.

On Thu, Oct 8, 2015, 01:26 Thierry Passeron notifications@github.com wrote:

Great, thanks @tdjordan https://github.com/tdjordan !

I am a bit ashamed that Nan reworded the NAN_METHOD "args" into "info" but I assume they did it for a reason. If anyone cares to enlighten me I'd be pleased.

While reviewing the code I kept thinking it should be good at some point to get rid of the old "#define"s used for arguments checking. There seems to be a better way of doing it with NanCheck().

Now, my other concern is whether I should merge this in master or in a separate branch. I'm open to discussion here.

— Reply to this email directly or view it on GitHub https://github.com/Orion98MC/node_rrd/pull/15#issuecomment-146431891.

Orion98MC commented 8 years ago

Hi Tom,

Thanks for the feedback.

Hence my concern about merging on master. It may be better to create a separate branch for node 4.x Not all of us have upgraded to 4.x. I for my part am still in 0.12.7

By the way, I assume you ran the tests and that they all pass am I right?

tdjordan commented 8 years ago

Test Results for node v0.12.7

$ make
mocha -t 10000 -w -G .

  rrd
    ✓ should load rrd
    ✓ should have .create function
    ✓ should have .update function
    ✓ should have .info function
    ✓ should have .last function
    ✓ should have .fetch function

  rrd DS
    ✓ should create a DS with rrd.DS
    ✓ should output DS spec

  rrd RRA
    ✓ should create a RRA with rrd.RRA
    ✓ should output RRA spec

  rrd create
    ✓ should create with DS and RRA objects
    ✓ should create with Array of DS and RRA objects
    ✓ should create with Objects for DS and RRA
    ✓ should create with Array of Objects for DS and RRA

  rrd update
    ✓ should update with timed values
    ✓ should update with timed values and fetch the same values (5002ms)

  Native bindings
    ✓ should load rrd_bindings
    Create
      ✓ should have a create function
      ✓ should create
    Update
      ✓ should have a update function
      ✓ should update
    Fetch
      ✓ should have a fetch function
      ✓ should fetch
    Last
      ✓ should have a last function
      ✓ should get last update time
    Info
      ✓ should have a info function
      ✓ should get RRD info

  27 passing (5s)

Test Results for node v4.1.1

$ make
mocha -t 10000 -w -G .

  rrd
    ✓ should load rrd
    ✓ should have .create function
    ✓ should have .update function
    ✓ should have .info function
    ✓ should have .last function
    ✓ should have .fetch function

  rrd DS
    ✓ should create a DS with rrd.DS
    ✓ should output DS spec

  rrd RRA
    ✓ should create a RRA with rrd.RRA
    ✓ should output RRA spec

  rrd create
    ✓ should create with DS and RRA objects
    ✓ should create with Array of DS and RRA objects
    ✓ should create with Objects for DS and RRA
    ✓ should create with Array of Objects for DS and RRA

  rrd update
    ✓ should update with timed values
    ✓ should update with timed values and fetch the same values (5003ms)

  Native bindings
    ✓ should load rrd_bindings
    Create
      ✓ should have a create function
      ✓ should create
    Update
      ✓ should have a update function
      ✓ should update
    Fetch
      ✓ should have a fetch function
      ✓ should fetch
    Last
      ✓ should have a last function
      ✓ should get last update time
    Info
      ✓ should have a info function
      ✓ should get RRD info

  27 passing (5s)

Test Results for iojs v3.3.1

$ make test
mocha -t 10000 -w -G .

  rrd
    ✓ should load rrd
    ✓ should have .create function
    ✓ should have .update function
    ✓ should have .info function
    ✓ should have .last function
    ✓ should have .fetch function

  rrd DS
    ✓ should create a DS with rrd.DS
    ✓ should output DS spec

  rrd RRA
    ✓ should create a RRA with rrd.RRA
    ✓ should output RRA spec

  rrd create
    ✓ should create with DS and RRA objects
    ✓ should create with Array of DS and RRA objects
    ✓ should create with Objects for DS and RRA
    ✓ should create with Array of Objects for DS and RRA

  rrd update
    ✓ should update with timed values
    ✓ should update with timed values and fetch the same values (5002ms)

  Native bindings
    ✓ should load rrd_bindings
    Create
      ✓ should have a create function
      ✓ should create
    Update
      ✓ should have a update function
      ✓ should update
    Fetch
      ✓ should have a fetch function
      ✓ should fetch
    Last
      ✓ should have a last function
      ✓ should get last update time
    Info
      ✓ should have a info function
      ✓ should get RRD info

  27 passing (5s)
Orion98MC commented 8 years ago

Ok so if I understand correctly, this will work for node 0.12.7 and 4.x as well. So there should be no need to merge on a separate branch.

Amazing work! Thanks!