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

upgrade to leveldb-1.19 #299

Merged
merged 7 commits into from
Sep 27, 2016
Merged

upgrade to leveldb-1.19 #299

merged 7 commits into from
Sep 27, 2016

Conversation

juliangruber
Copy link
Member

This is a work in progress pull request for #298

The build is currently failing.

@juliangruber
Copy link
Member Author

Build and tests now pass in my setup!

@juliangruber
Copy link
Member Author

We should create a .patch file for the floating port_uv.h patch

@juliangruber
Copy link
Member Author

Tests pass now on travis too. We can decide about adding new features in another pull request.

What do you all think? Should we do more manual testing? Manual inspection?

@ralphtheninja
Copy link
Member

💯

@chjj
Copy link
Contributor

chjj commented Aug 11, 2016

@juliangruber, great work.

Maybe we can merge it in to master, but hold off on publishing to npm for a little while? Either way, I'll be testing this thoroughly in practice for the next few days.

@ralphtheninja
Copy link
Member

@juliangruber I guess this would mean a 1.4.7 release right? When we're ready we should ping @mafintosh for builds. Maybe we should try to muster some prebuilts for windows as well.

@vweevers
Copy link
Member

Build and tests pass on Windows too. I can provide Windows prebuilds when the time comes.

@No9
Copy link
Contributor

No9 commented Aug 11, 2016

Thanks for this @juliangruber - builds and passes on my illumOS based distro
Also good on FreeBSD Current 11

@No9 No9 mentioned this pull request Aug 11, 2016
@ralphtheninja
Copy link
Member

Build and tests pass on Windows too. I can provide Windows prebuilds when the time comes.

👍

@ralphtheninja
Copy link
Member

Builds and tests pass on Debian Jessie.

@juliangruber
Copy link
Member Author

This sounds great!

I would just leave this here and wait for @chjj's tests results, then merge and release. (it's easy to by mistake release something when it's already in master)

By SemVer standard yeah it should be 1.4.7, but by "slightly personal meaningful semver" standard it should be 1.5.0

@mafintosh
Copy link
Member

Great job @juliangruber. I think we should leave the PR open for a few days and then merge + release it next week assuming nothing catastrophic appears.

@ralphtheninja
Copy link
Member

By SemVer standard yeah it should be 1.4.7, but by "slightly personal meaningful semver" standard it should be 1.5.0

+1 Makes sense.

@rvagg
Copy link
Member

rvagg commented Aug 23, 2016

This is interesting:

// EXPERIMENTAL: If true, append to existing MANIFEST and log files
// when a database is opened. This can significantly speed up open.
//
// Default: currently false, but may become true later.
bool reuse_logs;

We could enable an undocumented option for this to allow for experimentation. The one problem I see here is that it uses a new NewAppendableFile API in Env which would probably have to be implemented in port-libuv/env_win.cc because even though the documentation says: Users of Env (including the leveldb implementation) must be prepared to deal with an Env that does not support appending. I don't believe it does properly deal with that case!

@rvagg
Copy link
Member

rvagg commented Aug 23, 2016

PR LGTM except for two nits:

  • remove .travis.yml ?
  • chmod all files to 644, they are currently 755

rvagg and others added 2 commits August 23, 2016 21:42
@juliangruber
Copy link
Member Author

remove .travis.yml ?
chmod all files to 644, they are currently 755

@rvagg why?

@chjj
Copy link
Contributor

chjj commented Aug 24, 2016

Update: I've been using this branch in practice for a little while now. I've sync'd the entire blockchain with it. Works great.

@rvagg
Copy link
Member

rvagg commented Aug 25, 2016

remove .travis.yml ?
chmod all files to 644, they are currently 755

@rvagg why?

  1. Unnecessary churn, otherwise it's mostly a git rename and not an additional property change
  2. These files are not executable and it's incorrect for them to have execute bit set on them except for *.sh

@juliangruber
Copy link
Member Author

@rvagg this might sound stupid but, ...?

screen shot 2016-08-26 at 11 27 00

also, in current master the deps are already 755

@rvagg
Copy link
Member

rvagg commented Aug 26, 2016

find deps/leveldb -type f -exec chmod 644 '{}' \; && find deps/leveldb -type d -exec chmod 755 '{}' \;

I'm sure there's some neat umasky way of doing it but that's how I'd do it. Files 644, directories 755.

@juliangruber
Copy link
Member Author

gotcha, should all be addressed now!

@juliangruber
Copy link
Member Author

Final call for @chjj, I'm assuming everything looks good so far?

@ralphtheninja @mafintosh @vweevers how do we orchestrate publishing the prebuilds? Do you want to send yours to me so I can release and upload in one go?

@vweevers
Copy link
Member

@juliangruber I can get #309 going, once we reach consensus on:

Tagged commits trigger an upload to GitHub. Which means the AppVeyor job will be the first to create a GH release, whenever someone pushes a tag. Does anyone foresee problems with that? Alternatively, we can manually download artifacts from AppVeyor and attach them to a GH release. WDYT?

@chjj
Copy link
Contributor

chjj commented Sep 27, 2016

I can only speak from my own use here: this works perfectly on every x64 linux machine I've tried it on. Merging sounds good to me.

@ralphtheninja
Copy link
Member

@ralphtheninja @mafintosh @vweevers how do we orchestrate publishing the prebuilds? Do you want to send yours to me so I can release and upload in one go?

@juliangruber I'd suggest you sync with @mafintosh so he can push the button on linux and osx. Then we can build for arm and windows and update.

@ralphtheninja ralphtheninja merged commit 079dad7 into master Sep 27, 2016
@ralphtheninja ralphtheninja deleted the leveldb-1.19 branch September 27, 2016 08:21
@ralphtheninja
Copy link
Member

@juliangruber Can we bump some dependencies before releasing? I'm mainly thinking of the abstract-leveldown fix you made the other day.

@juliangruber
Copy link
Member Author

@ralphtheninja sure! do you want to make a pull request for that?

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.

7 participants