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

Add a hook to disable Git download and use Git distributed with OS #182

Open
mato opened this issue Apr 23, 2018 · 8 comments
Open

Add a hook to disable Git download and use Git distributed with OS #182

mato opened this issue Apr 23, 2018 · 8 comments

Comments

@mato
Copy link

mato commented Apr 23, 2018

Would it be possible to add a hook to dugite to disable any attempt at downloading a Git distribution at installation time and instead use the git binaries distributed with the system?

My need for this is twofold:

  1. I'm using dugite (actually surf-build which depends on it in the latest betas) on platforms for which you don't distribute native binaries and probably never will (various BSDs).
  2. Even on platforms where you do distribute native binaries, I'd prefer to use the system distribution rather than some random binary from the Internet.

I'm aware of LOCAL_GIT_DIRECTORY and GIT_EXEC_PATH, however those variables only have an effect at execution time, not at npm install time.

FWIW, you should be able to easily script detecting if Git is already correctly installed by attempting to run e.g. git --version and if successful using the output of git --exec-path.

@kittaakos
Copy link
Contributor

Just an idea; what if instead of disabling the download functionality (on npm i) we would try to link the local Git into the download folder if either the target platform is not supported or the download failed. So everything could remain the same. We could use find-git-exec to locate the executable. I do not know yet whether linking works/supported on all platforms, for instance, Windows.

@mato
Copy link
Author

mato commented Apr 24, 2018

As far as I know, Windows only supports symlinks on NTFS, and I'm not sure if Node.js has the appropriate WIN32 bindings to create them. However, this would still not address my 2nd point. Is there any implementation reason why it's problematic to just call out to the system git? (I'm not too familiar with how things are done in Node.js-land).

@j-f1
Copy link

j-f1 commented Apr 24, 2018

We want to provide a consistent Git interface, even if the user doesn’t have it installed or has a really old version.

@shiftkey
Copy link
Member

I've avoided using the system Git at install time because dugite was primarily designed to be distributed within apps - so using a known Git distribution and tracking the latest stable releases have been more of a focus. There's also a bunch of extra context here: desktop/desktop#3435 (comment)

Some other comments:

I'm using dugite (actually surf-build which depends on it in the latest betas) on platforms for which you don't distribute native binaries and probably never will (various BSDs).

I've actually got an open issue for this, if someone wants to get involved: desktop/dugite-native#82

however those variables only have an effect at execution time, not at npm install time.

As a workaround, I was hoping npm install --ignore-scripts was more granular, because the downloading of an external Git is done as a postinstall script, but it seems to ignore all scripts - which might impact other dependencies you're using.

@mato
Copy link
Author

mato commented Apr 27, 2018

[...] There's also a bunch of extra context here: desktop/desktop#3435 (comment)

I understand that your primary focus is the Desktop application, for which this approach makes sense.

I've actually got an open issue for this, if someone wants to get involved: desktop/dugite-native#82

Speaking as someone who runs CI for a multi-platform project where all the world's not a Linux container, this is a lot of work. There are no easy "CIaaS" setups for many of these platforms, unfortunately.

Given that an automatic solution to use the system Git requires careful consideration on your part, I'd like to suggest a minimal "I know what I'm doing, just do it" solution for use-cases such as mine. This could be as simple as setting e.g. DUGITE_USE_SYSTEM_GIT=1 at npm install time, which would disable the downloader and use whichever git is present on $PATH (possibly with a check at install time that there is indeed a git present, but I don't need that).

What do you think?

As a workaround, I was hoping npm install --ignore-scripts was more granular, because the downloading of an external Git is done as a postinstall script, but it seems to ignore all scripts - which might impact other dependencies you're using.

Yes, unfortunately the impact of that is unknown, and I'd prefer not to go further down the "what broke npm this time" rabbit hole.

@shiftkey
Copy link
Member

shiftkey commented May 4, 2018

Given that an automatic solution to use the system Git requires careful consideration on your part, I'd like to suggest a minimal "I know what I'm doing, just do it" solution for use-cases such as mine.

This seems fine.

This could be as simple as setting e.g. DUGITE_USE_SYSTEM_GIT=1 at npm install time, which would disable the downloader and use whichever git is present on $PATH (possibly with a check at install time that there is indeed a git present, but I don't need that).

We have an existing environment variable for you to be able to catch where dugite is downloaded to, so I'm open to a PR to support this. Here's the docs: https://github.com/desktop/dugite/blob/master/docs/environment-variables.md#installation

To make it clear this is install time, could we avoid USE here and maybe name it something like DUGITE_SKIP_DOWNLOAD_PACKAGE? For extra points, a warning and exit if git cannot be found on PATH like we do here would help with support on my end:

if (config.source === '') {
console.log(
`Skipping downloading embedded Git as platform '${process.platform}' is not supported.`
)
console.log(`To learn more about using dugite with a system Git: https://git.io/vF5oj`)
process.exit(0)
}

@kittaakos
Copy link
Contributor

so I'm open to a PR to support this

@shiftkey, are you still open for this change? We cannot include the following GPL-licensed files in the production:

Thank you!

@shiftkey
Copy link
Member

@kittaakos I'm open to reviewing a PR

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

4 participants