Skip to content
This repository has been archived by the owner on Dec 1, 2024. It is now read-only.

v0.10.x iojs support #146

Closed
wants to merge 4 commits into from
Closed

v0.10.x iojs support #146

wants to merge 4 commits into from

Conversation

mcollina
Copy link
Member

I backported some of the changes for making all tests of 0.10.2 pass in iojs and with nan 1.5.

It is not a PR in the general sense, if you are good with it you should release it as 0.10.3 so all modules depending on current level will be fixed.

heavyk and others added 4 commits January 22, 2015 18:00
Conflicts:
	package.json
	src/batch.cc
	src/database.cc
	src/database.h
	src/database_async.cc
	src/iterator.cc
	src/iterator_async.cc
	src/leveldown.cc
	src/leveldown.h
Conflicts:
	package.json
	src/database.cc
	src/database.h
	src/leveldown.cc
@ralphtheninja
Copy link
Member

@mcollina Very awesome! What is the biggest differences between 1.0 and 0.10?

@ralphtheninja
Copy link
Member

@mcollina Want me to take a look at the manual merge?

@juliangruber
Copy link
Member

awemazing!

@ralphtheninja
Copy link
Member

Ok. I'm taking a look at this merge conflict.

NanNull() only or NanNew(NanNull())?

<<<<<<< HEAD
      NanNull()
||||||| merged common ancestors
      NanNewLocal<v8::Value>(v8::Null())
=======
      NanNew(NanNull())
>>>>>>> v0.10.x-iojs

@mcollina
Copy link
Member Author

I think it should not be merged in, but rather the other way around,
backporting more macros into it.

In many ways, this is not a pull request, but rather a proposal for a
0.10.3 versions that works on both node and iojs. Please, contribute and
review it :).

@rvagg what do you think?
Il giorno gio 22 gen 2015 alle 22:21 Matteo Collina <
matteo.collina@gmail.com> ha scritto:

The
Il giorno gio 22 gen 2015 alle 21:58 Lars-Magnus notifications@github.com
ha scritto:

@mcollina https://github.com/mcollina Want me to take a look at the

manual merge?


Reply to this email directly or view it on GitHub
#146 (comment).

@ralphtheninja
Copy link
Member

Actually it's a good idea to have this merge. We will get the conflicts later anyway and might as well solve them now? :)

@ralphtheninja
Copy link
Member

@mcollina I added a new PR with the solved conflicts, so we could merge that one instead.

@mcollina
Copy link
Member Author

It is not supposed to be merged ever. We have an awesome 1.0 that looks
super good. I do not want to touch that, but I would try to avoid going to
update all modules that depends on levelup 0.18-0.19 and so on to the new
levelup to run inside iojs. We can just release it on npm as 0.10.3 with
tag 'legacy' or something similar so we do not bump latest. Let me say it
again: do not merge.

Also I had some conflicts even in node between leveldown and nodegit that
are using two very different nan version, and the version in 0.10.2 is
rather old.
Il giorno gio 22 gen 2015 alle 22:33 Lars-Magnus notifications@github.com
ha scritto:

@mcollina https://github.com/mcollina I added a new PR with the solved
conflicts, so we could merge that one instead.


Reply to this email directly or view it on GitHub
#146 (comment).

@mcollina
Copy link
Member Author

But please, test this one against levelup's unit tests, so we know if
everything is still super fine.
Il giorno gio 22 gen 2015 alle 22:42 Matteo Collina <
matteo.collina@gmail.com> ha scritto:

It is not supposed to be merged ever. We have an awesome 1.0 that looks
super good. I do not want to touch that, but I would try to avoid going to
update all modules that depends on levelup 0.18-0.19 and so on to the new
levelup to run inside iojs. We can just release it on npm as 0.10.3 with
tag 'legacy' or something similar so we do not bump latest. Let me say it
again: do not merge.

Also I had some conflicts even in node between leveldown and nodegit that
are using two very different nan version, and the version in 0.10.2 is
rather old.
Il giorno gio 22 gen 2015 alle 22:33 Lars-Magnus notifications@github.com
ha scritto:

@mcollina https://github.com/mcollina I added a new PR with the solved

conflicts, so we could merge that one instead.


Reply to this email directly or view it on GitHub
#146 (comment).

@ralphtheninja
Copy link
Member

Tests run fine on levelup. I'm using v1.0.3.

lms@lms-UX31A|00:49|~/src/leveldb-repos/node-levelup (master) $ npm run-script alltests 

> levelup@0.19.0 alltests /home/lms/src/leveldb-repos/node-levelup
> npm test && npm run-script functionaltests


> levelup@0.19.0 test /home/lms/src/leveldb-repos/node-levelup
> tap test/*-test.js --stderr

ok test/approximate-size-test.js ........................ 3/3
ok test/argument-checking-test.js ....................... 6/6
ok test/batch-test.js ................................. 15/15
ok test/binary-test.js ................................ 11/11
ok test/compression-test.js ............................. 4/4
ok test/copy-test.js .................................... 2/2
ok test/deferred-open-test.js ........................... 6/6
ok test/destroy-repair-test.js .......................... 7/7
ok test/encoding-test.js ................................ 7/7
ok test/get-put-del-test.js ............................. 9/9
ok test/idempotent-test.js .............................. 2/2
ok test/init-test.js .................................. 12/12
ok test/inject-encoding-test.js ......................... 5/5
ok test/json-test.js .................................... 5/5
ok test/key-value-streams-test.js ....................... 5/5
ok test/leveldown-substitution-test.js .................. 2/2
ok test/null-and-undefined-test.js .................... 13/13
ok test/open-patchsafe-test.js .......................... 4/4
ok test/optional-leveldown-test.js ...................... 5/5
ok test/read-stream-test.js ........................... 34/34
ok test/snapshot-test.js ................................ 2/2
ok test/write-stream-test.js .......................... 11/11
total ............................................... 170/170

ok

> levelup@0.19.0 functionaltests /home/lms/src/leveldb-repos/node-levelup
> node ./test/functional/fstream-test.js && node ./test/functional/binary-data-test.js && node ./test/functional/compat-test.js

***************************************************
RUNNING FSTREAM-TEST...
Guess what?? It worked!!
***************************************************
***************************************************
RUNNING BINARY-DATA-TEST...
Extracted tar file...
Opened database...
Piped data to database...
Finished comparing database entries...
Cleaning up...
No errors? All good then!
***************************************************
***************************************************
RUNNING COMPAT-DATA-TEST...
Extracted tar file...
Extracted tar file...
Opened database...
Finished comparing database entries...
Cleaning up...
No errors? All good then!
***************************************************
lms@lms-UX31A|00:49|~/src/leveldb-repos/node-levelup (master) $ iojs -e "console.log(require('./node_modules/leveldown/package.json').version)"
0.10.3

@rvagg
Copy link
Member

rvagg commented Jan 23, 2015

A bit heavier than I'd prefer a patch bump to be but I don't think I see anything in here that's of real concern.

If there's no -1's here then how about you put this in a "v0.10" branch and we'll let it live there and I'll publish.

The next question is, should we make this @latest in npm if we publish it as 0.10.3 and let 1.0.x fall off so that npm install leveldown gives you this one instead of 1.0.x and then switch that when we have levelup sorted out to match 1.0.x? I'm leaning towards that approach to avoid more of the "it doesn't work" issues.

@ralphtheninja
Copy link
Member

@rvagg That's a splendid idea and most of all it will buy some time.

@ralphtheninja
Copy link
Member

@mcollina I don't know why I wanted to merge in the first place. I didn't think of separate version tracks.

@ralphtheninja
Copy link
Member

@rvagg I've pushed a v0.10 branch.

@mcollina
Copy link
Member Author

👍 for bumbing latest too. But we should word this carefully in the READMEs.

@mcollina
Copy link
Member Author

Can anybody else in the team review it, so we publish it on NPM? Thanks!

@mcollina
Copy link
Member Author

@rvagg is there anything blocking you to release this? I can help if needed!

@@ -1,7 +1,7 @@
{
"name": "leveldown",
"description": "A Node.js LevelDB binding, primary backend for LevelUP",
"version": "0.10.2",
"version": "0.10.3",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should leave this to the releaser (me) btw, it's part of the npm version workflow and the commit gets signed as well.

@rvagg
Copy link
Member

rvagg commented Feb 7, 2015

it's crashy on 0.11 and 0.12:

$ dnt
node@v0.8.28 : FAIL wrote output to /tmp/leveldown-dnt-v0.8.28.out
node@v0.10.36: PASS wrote output to /tmp/leveldown-dnt-v0.10.36.out
node@v0.12.0 : FAIL wrote output to /tmp/leveldown-dnt-v0.12.0.out
node@v0.11.16: FAIL wrote output to /tmp/leveldown-dnt-v0.11.16.out
iojs@v1.0.0  : FAIL wrote output to /tmp/leveldown-dnt-v1.0.0.out
iojs@v1.1.0  : PASS wrote output to /tmp/leveldown-dnt-v1.1.0.out
Took 241s to run 6 versions of Node

@rvagg
Copy link
Member

rvagg commented Feb 7, 2015

fixed the crashiness in 6ea9962

borked a publish as 0.10.3, it got 1/2 way and the registry now thinks there's an unpublished 0.10.3, so published as 0.10.4

npm i level now works on io.js and 0.12

thanks for the hard work on this @ralphtheninja and @mcollina, and thanks for nagging me, I feel very guilty about the whole levelup/down situation right now, it's like my own personal Node 0.12 and that's quite shameful!

@rvagg rvagg closed this Feb 7, 2015
@rvagg rvagg deleted the v0.10.x-iojs branch February 7, 2015 11:23
@ralphtheninja
Copy link
Member

🍰

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

Successfully merging this pull request may close these issues.

5 participants