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

Npm: download the required binaries during installation #188

Merged
merged 17 commits into from
Jun 2, 2021

Conversation

aminya
Copy link
Contributor

@aminya aminya commented May 15, 2021

  • The binaries are now downloaded upon installation. This reduces the package size significantly, which is very important for open-source repositories.

  • Fixes finding the binaries on Windows inside the hooks. Previously the code fell back to using npx

Closes #43

@aminya aminya changed the title Npm: download the binaries inside postinstall script Npm: download the required binaries during installation May 15, 2021
.npm/postinstall.js Outdated Show resolved Hide resolved
@aminya aminya force-pushed the postinstall-download-binaries branch 6 times, most recently from 33280fb to 8af695a Compare May 15, 2021 15:48
@aminya aminya force-pushed the postinstall-download-binaries branch from c6598c3 to d0e8d2e Compare May 15, 2021 16:11
@Envek Envek requested review from Envek and HellSquirrel May 18, 2021 08:23
Copy link
Member

@Envek Envek left a comment

Choose a reason for hiding this comment

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

Hello! Thank you for your contribution and sorry for the late review!

Recently we had discussions on whether we should change packaging to download binaries on install or not and finally decided that we should. So your pull request is just in time!

However, there are some issues that should be addressed first:

  1. Download is broken on Linux and MacOS: file has wrong name due to incorrect filename extension detection and missing executable bit.

  2. In case if download failed, error message is unclear:

    [5/5] Rebuilding all packages...
    error /home/envek/test/node_modules/@arkweid/lefthook: Command failed.
    Exit code: 1
    Command: node postinstall.js
    Arguments: 
    Directory: /home/envek/test/node_modules/@arkweid/lefthook
    Output:
    events.js:292
          throw er; // Unhandled 'error' event
          ^
    
    Error: Response status was 404
        at ClientRequest.<anonymous> (/home/envek/evl.ms/test/node_modules/node-downloader-helper/dist/index.js:1:7153)
        at Object.onceWrapper (events.js:422:26)
        …
    

    To help users to understand what is going wrong (and to help us debug it), error message (Response status was 404 in this example) should be prefixed with details about what and when failed. Something like this:

    Failed to download lefthook_0.7.5_Linux_x86_64: Response status was 404 while fetching https://github.com/evilmartians/lefthook/releases/download/v0.7.5/lefthook_0.7.5_Linux_x86_64
    

.npm/bin/index.js Outdated Show resolved Hide resolved
.npm/postinstall.js Outdated Show resolved Hide resolved
.npm/postinstall.js Outdated Show resolved Hide resolved
@aminya
Copy link
Contributor Author

aminya commented May 19, 2021

Thank you. I addressed the comments.

@aminya aminya force-pushed the postinstall-download-binaries branch from 32ee663 to 743fde3 Compare May 19, 2021 19:21
@aminya aminya requested a review from Envek May 19, 2021 21:03
.npm/postinstall.js Outdated Show resolved Hide resolved
.npm/postinstall.js Outdated Show resolved Hide resolved
.npm/postinstall.js Outdated Show resolved Hide resolved
@Envek
Copy link
Member

Envek commented May 22, 2021

Yes, we know that. In that case other installation methods can be used instead.

I wish that NPM supported platform-specific packages, but in current form it is already way too heavy,

cmd/templates/hook.tmpl Outdated Show resolved Hide resolved
Co-authored-by: Andrey Novikov <envek@envek.name>
Envek pushed a commit that referenced this pull request May 22, 2021
This commit has been extracted from #188
@Envek
Copy link
Member

Envek commented May 22, 2021

I extracted everything about executable extension into separate commit and pushed it to master, so here we can focus on NPM package (no action needed).

@bbodenmiller
Copy link
Contributor

bbodenmiller commented May 22, 2021

Yes, we know that. In that case other installation methods can be used instead.

I wish that NPM supported platform-specific packages, but in current form it is already way too heavy,

Perhaps a net install (this PR) vs full install (current package) could be maintained then. Alternatively it appears there is options to handle this using OS dependencies: https://stackoverflow.com/questions/15176082/npm-package-json-os-specific-dependency

@aminya
Copy link
Contributor Author

aminya commented May 22, 2021

You could make a fork, and add proxy support similar to prebuild-install

@bbodenmiller
Copy link
Contributor

You could make a fork, and add proxy support similar to prebuild-install

Proxy support does not address organizations that forbid external downloads by policy and/or block external internet access other than via npm mirror.

Do what you want, I'm just telling you have this will break some workflows and it appears I'm not the first to raise this concern.

@aminya
Copy link
Contributor Author

aminya commented May 23, 2021

If the company blocks the internet, it should mock it using its proxy server. https://github.com/evilmartians/lefthook/releases/download URL can be simply redirected to a local server that hots the binaries. This is probably similar to how that company is mocking the URLs for the packages that use prebuild-install and node-pre-gyp (~6 million downloads a week).

@Envek
Copy link
Member

Envek commented May 24, 2021

We probably would like to support two packages (one with bundled executables and one that downloads them), but it requires changing how hook scripts invoke lefthook binary.

Envek pushed a commit that referenced this pull request Jun 1, 2021
This commit has been extracted from #188
Envek pushed a commit that referenced this pull request Jun 2, 2021
This commit has been extracted from #188
@Envek Envek merged commit aa26a08 into evilmartians:master Jun 2, 2021
@Envek Envek added this to the 0.8 milestone Jun 2, 2021
@Envek
Copy link
Member

Envek commented Jun 2, 2021

Thank you! We will release it soon. Maybe next week.

@aminya
Copy link
Contributor Author

aminya commented Jun 2, 2021

Thanks! 🎉

@privatenumber
Copy link

privatenumber commented Jun 14, 2021

Since this fetches binaries from GitHub, I want to raise the concern that this could interfere with environments that rely on a npm proxy registry (originally raised here).

I think this issue is more relevant than it appears. Many environments use a npm proxy in case npm goes down or becomes inaccessible (eg. CI deploys, China, etc). In the case where GitHub becomes unavailable, (which happens more than it should...), this will break npm install.

I think this can be easily addressed by publishing the binaries to npm and downloading from npm instead of GitHub (like playwright-chromium).

@aminya
Copy link
Contributor Author

aminya commented Jul 17, 2021

Thank you! We will release it soon. Maybe next week.

Any plan to release this?

@aminya
Copy link
Contributor Author

aminya commented Aug 6, 2021

@Envek

@Envek
Copy link
Member

Envek commented Aug 10, 2021

Sorry, hadn't a chance to prepare a release (it is pretty big compared to previous ones), so it is postponed until I will have a day or two to polish things out. Can't say any specific date or time now.

@aminya aminya mentioned this pull request Sep 28, 2021
@PikachuEXE PikachuEXE mentioned this pull request Sep 28, 2021
Envek added a commit that referenced this pull request Jun 7, 2022
Bundled NPM package was here before, installer was implemented in #188
@Envek
Copy link
Member

Envek commented Jun 7, 2022

It finally was released in 0.8.0 and now available as @evilmartians/lefthook-installer package.

Thank you very much for you contribution and sorry for such a long delay.

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.

4 participants