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

4.9.1 requires git to install #824

Closed
oznu opened this issue Mar 25, 2019 · 8 comments
Closed

4.9.1 requires git to install #824

oznu opened this issue Mar 25, 2019 · 8 comments

Comments

@oznu
Copy link

oznu commented Mar 25, 2019

Hello,

Commit 93ecd70 in release 4.9.1 adds git as a system dependency to successfully install the package.

Without git installed users can expect this error:

npm ERR! code ENOGIT
npm ERR! Error while executing:
npm ERR! undefined ls-remote -h -t ssh://[email protected]/mercadolibre/flexbuffer-node.git
npm ERR! 
npm ERR! undefined
npm ERR! No git binary found in $PATH
npm ERR! 
npm ERR! Failed using git.
npm ERR! Please check if you have git installed and in your PATH.

This might break existing automated builds, in my case it was a Docker image that previously had no need for git to be installed.

@luin
Copy link
Collaborator

luin commented Mar 25, 2019

Good caught! Just downgrade latest tag to 4.9.0. Will move the dependencies to npm in the next version.

@kerihenare
Copy link

This is solved in #826. However, I'm also looking at refactoring out flexbuffer entirely. It's a real small package, and ioredis only uses an even smaller portion of it.

@gabegorelick
Copy link

A better approach may be to refactor usages of flexbuffer to use Buffer and related JS builtins directly. Especially if it's not used much.

For example, https://github.com/luin/ioredis/blob/7736c1cf27af1cb991b95f592a5fbe89646facf6/lib/pipeline.ts#L306 appears to copy an entire Buffer just to write it back out. It may be possible to make stuff like this zero-copy by switching to using Buffers and/or TypedArrays directly (please correct me if I'm misunderstanding the code).

@dantman
Copy link

dantman commented Mar 29, 2019

If arg is large it may also be more efficient to convert the smaller strings to Buffers, push things onto an array instead of writing them to a flexbuffer, and use Buffer.concat to convert it to a single buffer all at once instead of repeatedly allocating slightly bigger buffers. If you want to super-optimize that, "\r\n" and "$" could be static buffers that are only allocated once and then shared in all usages (since you won't modify the buffers when making an array of them).

@stale
Copy link

stale bot commented Apr 28, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed after 7 days if no further activity occurs, but feel free to re-open a closed issue if needed.

@stale stale bot added the wontfix label Apr 28, 2019
@dantman
Copy link

dantman commented Apr 28, 2019

Still an important issue. Should not be closed until https://github.com/luin/ioredis/blob/master/package.json#L34 is changed.

@stale stale bot removed the wontfix label Apr 28, 2019
@stale
Copy link

stale bot commented May 29, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed after 7 days if no further activity occurs, but feel free to re-open a closed issue if needed.

@stale stale bot added the wontfix label May 29, 2019
@luin
Copy link
Collaborator

luin commented May 29, 2019

This issue has already been solved with https://github.com/luin/ioredis/releases/tag/v4.9.2. Forgot to close this.

@luin luin closed this as completed May 29, 2019
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

No branches or pull requests

5 participants