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

Upgrade snappy to 1.1.7 #522

Merged
merged 7 commits into from
Oct 31, 2018
Merged

Conversation

filoozom
Copy link
Contributor

Fixes #521

@filoozom
Copy link
Contributor Author

Lost one more line for some reason, strange. Shouldn't we move this to a Git Submodule now that I think about it?

@ralphtheninja
Copy link
Member

ralphtheninja commented Oct 25, 2018

Lost one more line for some reason, strange. Shouldn't we move this to a Git Submodule now that I think about it?

It's probably wise to do so now yes. But might not be completely straight forward. Needs some work to get paths right etc (e.g. snappy.gyp shouldn't use version numbers in path)

@ralphtheninja ralphtheninja requested a review from peakji October 25, 2018 16:36
@filoozom
Copy link
Contributor Author

filoozom commented Oct 25, 2018

This is actually great because we don't need to push the whole C++ codebase to npm for users that can just make use of prebuild packages.

Just looking for a way to remove all these dependencies after a npm run install, so that these dependencies are deleted once the package is installed as they won't be used anymore. Need to find a way that doesn't make developers lives harder by needing them do download to submodules on each build though.

.gitmodules Show resolved Hide resolved
@filoozom filoozom changed the title Upgrade snappy to 1.1.7 [WIP] Upgrade snappy to 1.1.7 Oct 25, 2018
@filoozom
Copy link
Contributor Author

filoozom commented Oct 25, 2018

I'd like to add something like this to package.json:

{
  "scripts": {
    "make": "git submodule update --init && node-gyp rebuild",
    "install": "prebuild-install || npm run make && git submodule deinit .",
  }
}

(I chose make instead of build because if I add a non-empty build script it automatically runs prebuild for some reason. Maybe this can be fixed?)

This would be nice for users because they wouldn't end up with a few megabytes of useless C++ dependencies, but would require developers and CI to download all git submodules twice. Any ideas?

Except for that, this PR is ready. Maybe I should work on that on another PR actually.

@filoozom filoozom changed the title [WIP] Upgrade snappy to 1.1.7 Upgrade snappy to 1.1.7 Oct 25, 2018
@ralphtheninja
Copy link
Member

Actually, I don't think we should do the git submodule update-dance during the install phase. This will cause problems when people are installing it from npm. The code should still be published together with the package, so everything is in the tarball from npm.

@filoozom
Copy link
Contributor Author

That's a good point. Wouldn't work if installing on a system without git. Guess there's actually no way to make the package smaller after an install then. 🙁

At least this should make it easier to update upstream dependencies (cd deps/snappy/snappy; git checkout $version, commit the thing and there you go).

@ralphtheninja
Copy link
Member

At least this should make it easier to update upstream dependencies (cd deps/snappy/snappy; git checkout $version, commit the thing and there you go).

Yes, it's a huge improvement when it comes to maintenance :) We should do this with all code that's vendored, e.g. leveldb and rocksdb as well.

@filoozom
Copy link
Contributor Author

@ralphtheninja I'm already on it. 😉 Just making sure that all versions are exactly the same and that I'm not overriding a patch or something.

@filoozom
Copy link
Contributor Author

filoozom commented Oct 25, 2018

Okay, this one should be good to go. 👍

Maybe push a test release to check if everything works on npm?

@ralphtheninja
Copy link
Member

Maybe push a test release to check if everything works on npm?

Will test tomorrow.

@ralphtheninja ralphtheninja self-assigned this Oct 25, 2018
@filoozom
Copy link
Contributor Author

Hey @ralphtheninja, have you had some time to test this out? If so, I could start working on moving all dependencies to git modules.

Copy link
Member

@ralphtheninja ralphtheninja left a comment

Choose a reason for hiding this comment

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

Looks fine, but I think we should document this in README for devs. Someone cloning the repo should probably clone it recursively (or do the git submodule dance)

README.md Outdated Show resolved Hide resolved
Co-Authored-By: filoozom <[email protected]>
@ralphtheninja ralphtheninja merged commit cdf0f1a into Level:master Oct 31, 2018
@ralphtheninja
Copy link
Member

Nice!

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.

4 participants