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

Migrate wp-env to use simple-git instead of nodegit #26660

Closed
mkaz opened this issue Nov 3, 2020 · 9 comments · Fixed by #28848
Closed

Migrate wp-env to use simple-git instead of nodegit #26660

mkaz opened this issue Nov 3, 2020 · 9 comments · Fixed by #28848
Labels
[Priority] Low Used to indicate that the issue at hand isn't a top priority to address and can be handled later [Tool] Env /packages/env

Comments

@mkaz
Copy link
Member

mkaz commented Nov 3, 2020

With the new LTS upgrade (14.15.0), the nodegit prebuilt binary is not yet available and requires being built from source. This is an extra complication to require everyone using wp-env to have all the dependencies installed.

This also highlighted that we probably don't need all the features of nodegit and could probably switch over to use simple-git that does have a requirement that git be installed locally, but that is safe tto assume and require for the project, since you need git to check out the project.

@mkaz mkaz added the [Tool] Env /packages/env label Nov 3, 2020
@gziolo
Copy link
Member

gziolo commented Nov 3, 2020

It might also work if we upgrade to the latest version of nodegit:

https://github.com/nodegit/nodegit/releases/tag/v0.27.0

Prebuilds for Node 14, deprecate Node 8

@mkaz
Copy link
Member Author

mkaz commented Nov 10, 2020

I'm not sure how important it is or not, but it looks like converting to simple-git will lose the progress indicator when cloning the repo. The current NodeGit provides a callback with progress, simple-git does not have that feature.

Used here: https://github.com/WordPress/gutenberg/blob/master/packages/env/lib/download-sources.js#L111-L121

With the upgrade in #26712 this issue is fairly low priority, I think it would lighten the overall package, so probably still worth doing.

@mkaz mkaz added the [Priority] Low Used to indicate that the issue at hand isn't a top priority to address and can be handled later label Nov 10, 2020
@gziolo
Copy link
Member

gziolo commented Nov 11, 2020

I see this issue in simple-git repository:

steveukx/git-js#265

It isn't impossible but this would require some time investment to refactor. I'm wondering how much time it takes to install each library and what is the size difference on the disk. Maybe it's fine to keep nodegit.

@noahtallen
Copy link
Member

I agree that it's probably not a big issue for us. One nice thing is that wp-env can be used outside of Gutenberg, so it might not be safe to assume git is available. (Though to be fair, I do agree it's very likely available most places.)

@noahtallen
Copy link
Member

I'm thinking we should get this done. When installing via npm i -g @wordpress/env, it continues failing if I don't have a specific Node version available (nvm use 14 makes it work.) I think it's worth supporting current NodeJS, as well as the LTS version, and as far as I can tell, nodegit is the only thing preventing us from doing so. If they updated more quickly to address this issue, maybe we could stick with it... But as is, I don't see a new release since the one we have installed.

But yeah, I agree that simple git would be simpler (ha) for us to maintain.

@ockham
Copy link
Contributor

ockham commented Feb 5, 2021

Here's one more reason to migrate to simple-git (#28741 (comment)):

I think the underlying issue is that nodegit is a rather low-level API wrapper for libgit2 -- where things like checkout have a rather different meaning from their git CLI counterparts we all are used to.

I've noticed that for the GB release tools, we use a different git library, simple-git:

const SimpleGit = require( 'simple-git/promise' );

OTOH, nodegit is only used by wp-env's download script.

So I think that he fact that nodegit is low-level and complex, and that there's simple-git, and that we're already using it 😄 is a good indicator that we might want to migrate the wp-env's download script to simple-git, too 😬

@ockham
Copy link
Contributor

ockham commented Feb 5, 2021

As for the progress meter, there's another upstream issue which seems to have more information (and a recipe?): steveukx/git-js#529

@noahtallen
Copy link
Member

nice. I also don't think that progress should block this move 😅

Btw, I did start poking around at switching, but never pushed the branch. I'll see if I can find it today :)

@noahtallen
Copy link
Member

Here's what I started a couple weeks ago: #28804. I didn't make a ton of progress, but I think it at least will clone the repo. Needs a lot of polished around other cases though. I don't know how much time I have to continue working on it in the next week, but feel free to continue with it if you have time ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Priority] Low Used to indicate that the issue at hand isn't a top priority to address and can be handled later [Tool] Env /packages/env
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants