Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NAN 1.5 WIP #216

Merged
merged 1 commit into from
Jan 14, 2015
Merged

NAN 1.5 WIP #216

merged 1 commit into from
Jan 14, 2015

Conversation

kkoopa
Copy link
Collaborator

@kkoopa kkoopa commented Nov 6, 2014

This branch will become NAN 1.5.
Added Asyncworker #207 and new NanNew #210. Needs more testing with actual addons.
Now split into many files. Seems to work so far. Good news for nauv.h

@kkoopa kkoopa force-pushed the onepointfive branch 2 times, most recently from 5fafced to 3390d07 Compare November 7, 2014 20:36
};

template <>
class Factory<v8::FunctionTemplate> : public FactoryBase<v8::FunctionTemplate> {

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

@kkoopa kkoopa force-pushed the onepointfive branch 2 times, most recently from 7bb1993 to a9c3ca9 Compare November 8, 2014 18:35
@kkoopa
Copy link
Collaborator Author

kkoopa commented Nov 20, 2014

Fixed the issue with taking the address of rvals.

@kkoopa
Copy link
Collaborator Author

kkoopa commented Dec 7, 2014

ETA: Before the end of the year.

@rvagg
Copy link
Member

rvagg commented Dec 7, 2014

Someone pinged me on IRC this morning commenting that we're not compatible with io.js, it'd be good to get there soon if this is the case (I haven't had time to confirm yet)

@rvagg
Copy link
Member

rvagg commented Dec 7, 2014

Also, it would seem that adding @agnat as a collaborator is in order too, unless anyone has any objections.

@kkoopa
Copy link
Collaborator Author

kkoopa commented Dec 7, 2014

trying to exec 'node' to find the location, yes. It's the same as when debian renamed the binary. It will be an immense pain in the ass to adjust and have something working with all of them, if at all possible. I think Windows causes problems fot this. io.js should at least have an alias called node, otjerwise I tjink we have to have three different NANs.

On December 7, 2014 11:50:21 PM EET, Rod Vagg [email protected] wrote:

Someone pinged me on IRC this morning commenting that we're not
compatible with io.js, it'd be good to
get there soon if this is the case (I haven't had time to confirm yet)


Reply to this email directly or view it on GitHub:
#216 (comment)

@rvagg
Copy link
Member

rvagg commented Dec 7, 2014

If that's the only problem then fine, the aim is for iojs to be aliased to "node" anyway. I was concerned it might be about the V8 version discrepancy.

@kkoopa
Copy link
Collaborator Author

kkoopa commented Dec 7, 2014

Oh yes, do that. I think you as owner is the only who can do it. I can update the readme and such. I still have to do some practical testing of this, but have a very packed schedule now before christmas, that's why I estimate three more weeks.

On December 7, 2014 11:52:47 PM EET, Rod Vagg [email protected] wrote:

Also, it would seem that adding @agnat as a collaborator is in order
too, unless anyone has any objections.


Reply to this email directly or view it on GitHub:
#216 (comment)

@kkoopa
Copy link
Collaborator Author

kkoopa commented Dec 7, 2014

Don't know which version is where, but I don't think it's any more than that we have to execute 'node' to find the location of NAN, the magic binding.gyp include line.

On December 8, 2014 12:01:01 AM EET, Rod Vagg [email protected] wrote:

If that's the only problem then fine, the aim is for iojs to be aliased
to "node" anyway. I was concerned it might be about the V8 version
discrepancy.


Reply to this email directly or view it on GitHub:
#216 (comment)

@rvagg
Copy link
Member

rvagg commented Dec 7, 2014

Protip: always "two weeks"--engineer speak for some vague point in the future you can't really be sure of. A secret code that engineers understand but management get to take seriously.
e.g. 0.12 has been "two weeks" for nearly a year.

@agnat
Copy link
Collaborator

agnat commented Dec 8, 2014

Two points:

  1. Fortunately we are not under any pressure on this. @kkoopa, if your schedule is packed already... take a few days, enjoy christmas and we put her out early next year. No rush...
  2. Regarding the include directory...

Lets add the following file to nan:

nan.gypi
{ 'include_dirs': ['.'] }

... and tell people to use it like this:

{ 'targets': [
  { 'target_name': 'foo'
  , 'sources': ['foo.cpp']
  , 'includes': ['node_modules/nan/nan.gypi']
  }
]}

Unless I'm missing something that works on all platforms under all circumstances. It also gives us more flexibility. We now really can tamper with the build system... ;)

@rvagg
Copy link
Member

rvagg commented Dec 8, 2014

  , 'includes': ['node_modules/nan/nan.gypi']

Unfortunately this won't be good enough, we don't have guarantees that it'll be located in that directory. If there are multiple projects using NAN and the dependency ranges allow it, it'll be installed in a common path, possibly something like ../nan. This is why we have the "<!(node -e \"require('nan')\")", to figure out where it's located.

@agnat
Copy link
Collaborator

agnat commented Dec 8, 2014

Really? I've never seen that. Who is responsible for this magic sharing? npm? Could you point out the documentation?

I'm kind of confused right now. node/npm did break the "un-share everything"-paradigm?

@rvagg
Copy link
Member

rvagg commented Dec 8, 2014

deps are shared if the dependency ranges specified in package.json allow for it

try this in a new directory:

$ npm install [email protected] modest
...
├─┬ [email protected]
...
│ ├─┬ [email protected]
│ │ └── [email protected]
...
└── [email protected]

Note how contextify wants nan@~1.3.0 but since we also have it at the top level and it matches the range then it doesn't bother installing an new version of it to save duplication—but also the node_modules/nan path isn't assured for contextify, hence the path-lookup hack.

@agnat
Copy link
Collaborator

agnat commented Dec 8, 2014

Hmk. Thanks for the explanation. I'd say it isn't a hack then. Under these circumstances it's the only way....

It still might be an idea to switch to including a gypi file at some point. However, this is 1. OT and 2. even less pressing than 1.5. ;)

@rvagg
Copy link
Member

rvagg commented Jan 13, 2015

@kesla let's get this out, [email protected] is about to be released so let's sync with that. If you need any help with release process let me know!

@kesla
Copy link
Contributor

kesla commented Jan 13, 2015

I guess you meant pinging @kkoopa?

@rvagg
Copy link
Member

rvagg commented Jan 13, 2015

derp, yes, sorry @kesla, you were the first name to pop up, @kkoopa was who I meant

@kkoopa
Copy link
Collaborator Author

kkoopa commented Jan 13, 2015

What about the functions moved from v8::V8 to v8::Isolate? We'll have to do a 1.6 very soon otherwise to cover the v8 discrepancies.

On January 13, 2015 11:03:48 PM EET, Rod Vagg [email protected] wrote:

@kesla let's get this out, [email protected] is about to be released so let's
sync with that. If you need any help with release process let me know!


Reply to this email directly or view it on GitHub:
#216 (comment)

@rvagg
Copy link
Member

rvagg commented Jan 13, 2015

what version of V8 is that in? io.js will be on 3.31 for a while I think, we were burnt already with changes to ES6 classes and their announced removal for 3.31 before it goes final so it's likely there will be a bit of conservatism for the next month or two at least.

@kkoopa
Copy link
Collaborator Author

kkoopa commented Jan 13, 2015

3.29 perhaps... Not quite sure. It's later than what's in 0.11.14, but in joyent/0.12 branch as of now and whatever is used in atom-shell nowadays. I specifically know of the previously mentioned ContextDisposedNotification and I planned on getting it fixed in this release. It's a small addition, but should look through all other functions too to see what else has moved.

Regarding cutting a release, it went quite well last time, apart from npmjs not receiving README.md properly for reasons unknown to me. Maybe due to having an old npm client. I have since updated npm. Any ideas what happened there?

I can push a release out in the morning, say in 10-12 hours. Would like 12 more to tie up these loose ends and avoid releasing 1.5 only to follow with 1.6 a couple days later. How does that sound?

On January 13, 2015 11:29:54 PM EET, Rod Vagg [email protected] wrote:

what version of V8 is that in? io.js will be on 3.31 for a while I
think, we were burnt already with changes to ES6 classes and their
announced removal for 3.31 before it goes final so it's likely there
will be a bit of conservatism for the next month or two at least.


Reply to this email directly or view it on GitHub:
#216 (comment)

@rvagg
Copy link
Member

rvagg commented Jan 13, 2015

re npm, I have no idea, npm looks at .npmignore if it exists, .gitignore otherwise, then it looks at the files on disk and packages those. If you have the problem again, just do a patch release, no big deal to do patch releases to paper over publish problems.

timing is fine by me, whatever works for you, just expect people to start filing bugs against projects, and NAN, for io.js support today.

@rvagg rvagg mentioned this pull request Jan 14, 2015
@kkoopa
Copy link
Collaborator Author

kkoopa commented Jan 14, 2015

What's with the failed build due to bad node-gyp checksum?
Can you add docker tests for io.js here? I meant travis.

@rvagg
Copy link
Member

rvagg commented Jan 14, 2015

odd error from travis, I have some work to do to make dnt io.js compatible yet but I can have a go

@rvagg
Copy link
Member

rvagg commented Jan 14, 2015

initial results:

$ dnt
[email protected]: PASS wrote output to /tmp/nan-dnt-v0.10.34.out
[email protected]: PASS wrote output to /tmp/nan-dnt-v0.11.14.out
[email protected]: PASS wrote output to /tmp/nan-dnt-v1.0.1-release.out
Took 24s to run 3 versions of Node

io.js supporting branch of dnt is rvagg/archived-dnt#14

I don't have images built for the other Node versions we should be testing but I'll start that build process not, I just doubt it'll all be done before I have to go to bed. I'm confident with the above results, no warnings or anything in that output file, very clean. I'm 👍 on a release.

@kkoopa
Copy link
Collaborator Author

kkoopa commented Jan 14, 2015

Good, good. I'll push it out like this when I get home then. Just have to
update the changelog. Those other changes I listed get to wait until 1.6, in a
week or two. They are quite minor.

On Wednesday 14 January 2015 04:44:33 Rod Vagg wrote:

initial results:

$ dnt
[email protected]: PASS wrote output to /tmp/nan-dnt-v0.10.34.out
[email protected]: PASS wrote output to /tmp/nan-dnt-v0.11.14.out
[email protected]: PASS wrote output to /tmp/nan-dnt-v1.0.1-release.out
Took 24s to run 3 versions of Node

io.js supporting branch of dnt is rvagg/archived-dnt#14

I don't have images built for the other Node versions we should be testing
but I'll start that build process not, I just doubt it'll all be done
before I have to go to bed. I'm confident with the above results, no
warnings or anything in that output file, very clean. I'm 👍 on a
release.


Reply to this email directly or view it on GitHub:
#216 (comment)

@kkoopa kkoopa merged commit 39f85f4 into master Jan 14, 2015
@rvagg
Copy link
Member

rvagg commented Jan 14, 2015

[email protected]: PASS wrote output to /tmp/nan-dnt-v0.11.14.out
[email protected]: PASS wrote output to /tmp/nan-dnt-v0.10.34.out
[email protected]: PASS wrote output to /tmp/nan-dnt-v0.11.13.out
[email protected]: PASS wrote output to /tmp/nan-dnt-v0.10.33.out
[email protected]: PASS wrote output to /tmp/nan-dnt-v0.10.32.out
[email protected]: PASS wrote output to /tmp/nan-dnt-v0.10.30.out
[email protected]: PASS wrote output to /tmp/nan-dnt-v0.10.31.out
[email protected]: PASS wrote output to /tmp/nan-dnt-v0.10.29.out
[email protected]: PASS wrote output to /tmp/nan-dnt-v0.10.28.out
[email protected]: PASS wrote output to /tmp/nan-dnt-v0.10.26.out
[email protected]: PASS wrote output to /tmp/nan-dnt-v0.10.25.out
[email protected]: PASS wrote output to /tmp/nan-dnt-v0.10.24.out
[email protected]: PASS wrote output to /tmp/nan-dnt-v0.10.22.out
[email protected]: PASS wrote output to /tmp/nan-dnt-v0.10.23.out
[email protected]: PASS wrote output to /tmp/nan-dnt-v0.10.21.out
[email protected]: PASS wrote output to /tmp/nan-dnt-v0.10.20.out
[email protected]: PASS wrote output to /tmp/nan-dnt-v0.10.19.out
[email protected] : PASS wrote output to /tmp/nan-dnt-v0.8.28.out
[email protected] : PASS wrote output to /tmp/nan-dnt-v0.8.27.out
[email protected] : PASS wrote output to /tmp/nan-dnt-v0.8.26.out
[email protected] : PASS wrote output to /tmp/nan-dnt-v0.8.24.out
[email protected]: PASS wrote output to /tmp/nan-dnt-v1.0.1-release.out
Took 113s to run 22 versions of Node

@kkoopa
Copy link
Collaborator Author

kkoopa commented Jan 14, 2015

Nice, very nice indeed.

@rvagg rvagg deleted the onepointfive branch January 14, 2015 20:55
@rvagg
Copy link
Member

rvagg commented Jan 14, 2015

agreed, well done @kkoopa!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants