Download binary on npm postinstall hook #43

fleischie · 2019-08-02T12:09:17Z

As was discussed in issue #16 the lefthook npm package contains all the binaries, this postinstall hook downloads only the appropriate binary for the host OS (or in case of CI: none).

Note: I submit my PR in accordance to #16 (comment).
Note 2: I did not run any go/ruby command as this PR mainly changes the postinstall script in the .npm directory. If this is still necessary I will update my PR, just let me know what is missing. ✌️

Arkweid · 2019-08-02T16:03:42Z

/cc @HellSquirrel
#16

HellSquirrel · 2019-08-02T18:05:41Z

Thanks! I will test it in a few days

HellSquirrel

@fleischie
I tested this solution and found some problems. See comments below

.npm/postinstall.js

HellSquirrel · 2019-08-07T23:11:16Z

.npm/postinstall.js

+
+function downloadBinary(os, callback) {
+ // construct single binary file path
+ const binaryPath = join(process.cwd(), 'lefthook');


process.cwd() is our current working directory. Consider some examples
npm i @arkweid/lefhook -- it will upload the binary to the project root
cd src && npm i @arkweid/lefhook -- it will upload the binary to 'src'
We want the binary to be uploaded to ./node_modules/@arkweid/lefhook/...

Oh, whoops. I guess that was just the leftover binaryPath from my local tests. Will update immediately.

process.cwd() should also give us the base path of the project, as the main function calls process.chdir(process.env.INIT_CWD).

But if you want I can rewrite this, so that both the downloadBinary function takes process.env.INIT_CWD into consideration for the path, as well as call process.chdir(process.env.INIT_CWD) inside installGitHooks.

We have two problems:

process.env.INIT_CWD refers to the current folder. (It could be a subfolder of the root folder.) So we need to look for node_modules recursively

Some older versions of yarn don't set this variable at all

No, you shouldn't search the folder node_modules at all (e.g., name of "node_modules" folder can be changed). There is simple and steady solution: https://github.com/levenkov/lefthook/pull/1/files#diff-5fe0be6c71fdcee66beee780a898073fR167

Download it in preinstall hook.

preinstall hook executed in the package directory.

It's always true, that if you placed downloaded binary in ./bin in the preinstall hook, this binary after install stage will be in the right place (usually here: <project dir>/node_modules/@arkweid/lefthook/bin/...).

I almost done the work, but was overwhelmed by the deadline :)

I was under the impression that the INIT_CWD is set to the base path of the newly installed package (see https://github.com/yarnpkg/yarn/blob/cfb560e1b2e37ed5cfe5d2374a806ac3ae512c11/src/util/execute-lifecycle-script.js#L40 similar for npm), hence INIT_CWD for lefthook would always be <project-dir>/node_modules/@Arkweid/lefthook.

(Also the change was (according to GitHub) over a year ago, so I would assume most users have a current enough yarn/npm installation)

So tl;dr: As long as we don't process.chdir or cd <somewhere> && ... in the postinstall script, we should be able to assume we are in <project-dir>/node_modules/@Arkweid/lefthook, no?

(Also the change was (according to GitHub) over a year ago, so I would assume most users have a current enough yarn/npm installation)

It's better to support as earlier as you can version of node. It's common place to support >=6.0.

INIT_CWD is working after 8.

@levenkov @HellSquirrel

I honestly don't know how to fix this INIT_CWD issue. I tried to use the ./dir approach (i.e. const path = process.env.INIT_CWD || path.resolve(".");), but in my locally installation tests, this always resolved to the path the script resides in (i.e. <path>/lefthook/.npm, instead of <path>/tmp where I called npm install ../lefthook/.npm from).

Maybe you can review the latest PR version and give me some pointers.

@fleischie I'll see that in 1-2 days. Maybe I'll try to change the approach in your code to mine and suggest small patch. I don't claim to replace all your work, just this problem.

HellSquirrel · 2019-08-11T14:49:39Z

Here how you can load unzip and execute lefthook binary on mac

const https = require("https");
const fs = require("fs");
const zlib = require("zlib");

const url =
  "https://github.com/Arkweid/lefthook/releases/download/v0.6.3/lefthook_0.6.3_MacOS_x86_64.gz";

const fileName = "lefthook-local";

https
  .get(url, res => {
    if (res.statusCode === 302) {
      const originalUrl = res.headers.location;
      console.log("redirect, redirect");

      https
        .get(originalUrl, res => {
          if (res.statusCode === 200) {
            res
              .pipe(zlib.createGunzip())
              .pipe(fs.createWriteStream(fileName))
              .on("finish", res =>
                fs.chmod(fileName, 755, err => {
                  if (err) {
                    console.log("error changing rights");
                  }

                  console.log("done");
                })
              );
          }
        })
        .on("error", e => {
          console.log("error after redirect!", e);
        });
    }
  })
  .on("error", e => {
    console.error(e);
  });

HellSquirrel · 2019-08-11T15:14:52Z

For Windows we should change the url variable to https://github.com/Arkweid/lefthook/releases/download/v0.6.3/lefthook_0.6.3_Windows_x86_64.gz

fleischie · 2019-08-13T12:57:28Z

(I will have time (maybe today but definitely) on thursday to continue working on this PR. Sorry for the inconvenience. 🙇 )

ai · 2019-09-26T01:05:28Z

Is it ready?

fleischie · 2019-09-26T07:42:46Z

I finished with the PR for now (as in "I tried to implement the feature while considering all raised concerns").

I think someone should (or maybe also wanted to) test the current state, as I do not know how to test the postinstallation hook from an actual npm install/yarn add. Let me know, if the PR still needs work to be done.

HellSquirrel · 2019-09-26T10:16:52Z

I'm still going to test it :) I can get here in a week or two

ai · 2019-09-26T10:26:12Z

There is a simple way to test postinstall

npm pack to get tgz file
Use link to this tar file in demo project npm install ../lefthook/lefthook.tgz

yumauri · 2019-10-23T09:19:32Z

Still there? :)

fleischie · 2019-10-23T09:38:06Z

Oh hey, yes. I actually overlooked the last message from Andrey, so I will check today/tomorrow on some systems whether it works ok, and then report back. ✌️

fleischie · 2019-10-28T09:51:32Z

I did now test my PR on mac and Windows 7 VM:
The downloading and calling of lefthook install -f seems to work alright. Unfortunately as my VM image is 32-bit, I couldn't verify that the complete process works (edit 2: on Windows that is).

It does for mac (if I change the index.js appropriately).

Edit: I will hence update the PR, and need to test on my Windows machine at home (if I find the time that is).

fleischie · 2019-10-28T10:02:51Z

I also rebased the PR on the latest master branch, removed the [WIP] from the commit-in-question, and some further improvements to the postinstall script:

only allow 64bit downloads on windows (there are no 32bit executables in the releases tab),
remove the pre-build executables from the binary folder, and
consolidate the naming of the executable (rename the downloaded executable lefthook/lefthook.exe for MacOS/Linux resp. Windows), and call them by name/platform from index.js.

yumauri · 2019-10-28T10:14:07Z

I'm not sure, but is it possible to point package.json's bin.lefthook property right into binary, instead of index.js, to eliminate node spawning for simply choose right binary?

UPD: actually, that is what actually mentioned in #16 as second point

Try to fetch the binary files on postinstall for each supoorted OS and if not CI. This should reduce the npm package size (as you only ever download one binary for your host system).

- Add function to dynamically return path of `lefthook` binary depending on `os`, package `version`, and architecture, - add extra redirection-download step for GitHub releases (as they are served from AWS, which is dynamically redirected on request), and - add more event-based file-handling (e.g. unlinking in 'close'-handler, calling the callback on 'end'-event, etc.).

If `process.env.INIT_CWD` is not set fall back to `resolve(".")` to try to emulate the behavior.

Exit early and notify user of issue, because currently there is only a windows 64bit version of lefthook.

- Remove pre-build binaries in `bin/` folder, - rename downloaded executable to `lefthook` (or `lefthook.exe` on windows), and - call the appropriate executable by name (and platform) in `index.js`.

fleischie · 2019-11-11T10:14:27Z

Updated the PR again to fit snugly on the latest master.

@yumauri I am uncertain whether package.json#bin.lefthook can be the lefthook binary directly, as firstly the docs on that field give the impression that it must be a node script, and secondly when i attempted to do it npm (at least) complained about a missing script, although the binary existed. 🤷‍♂

fleischie · 2019-11-11T10:26:05Z

Amendum: This PR works on the assumption, that binaries are always available for the most recent version as a gzipped package. That's why there is a commit aborting on 32bit Windows.

I understand that this is an added burden on the maintainer, as there are more and more diverse packages to be produced. The other solution would be to handle the zips as well, but require an additional (potentially heavy) node zip-library, which I tried to avoid until now.

Let me know what you think.

yumauri · 2019-11-11T10:31:41Z

Github actions are publicly available ;) Theoretically it is possible to make an action to build and upload binaries somewhere on every push

ai · 2020-01-15T00:57:49Z

@yumauri I am uncertain whether package.json#bin.lefthook can be the lefthook binary directly, as firstly the docs on that field give the impression that it must be a node script, and secondly when i attempted to do it npm (at least) complained about a missing script, although the binary existed. man_shrugging

You can test npm features by installing npm package from local file npm i test@../test.

Github actions are publicly available ;) Theoretically it is possible to make an action to build and upload binaries somewhere on every push

Yeap, .gz files is not a problem. We will find a solution for them.

ringods · 2020-02-26T14:01:46Z

Please, please, please do NOT merge this. Not everybody is working in an environment where all machines have direct internet connectivity!

I have customers in highly regulated environments where their working machines are isolated from the internet. Development is done with an internal package repository, acting as a proxy for upstream repositories. All package downloads come from this proxy.

The current situation, with the binaries included in the lefthook package, allow these customers to work with lefthook correctly. If you change it to downloading during the postinstall, their setups will break and they have no way to work around it as you hardcode the download URL.

Making the URL configurable also pushes a lot of chore onto the users as they have to copy the binaries, put them on an internal site and communicate the link. Quite error prone and not usage friendly.

To wrap up: can you please leave it as it is?

ai · 2020-02-26T17:31:33Z

Yeap, I changed my mind. Let's close it.

aminya · 2021-05-10T03:58:27Z

Please, please, please do NOT merge this. Not everybody is working in an environment where all machines have direct internet connectivity!

I have customers in highly regulated environments where their working machines are isolated from the internet. Development is done with an internal package repository, acting as a proxy for upstream repositories. All package downloads come from this proxy.

The current situation, with the binaries included in the lefthook package, allow these customers to work with lefthook correctly. If you change it to downloading during the postinstall, their setups will break and they have no way to work around it as you hardcode the download URL.

Making the URL configurable also pushes a lot of chore onto the users as they have to copy the binaries, put them on an internal site and communicate the link. Quite error prone and not usage friendly.

To wrap up: can you please leave it as it is?

Well, not everyone has high-speed internet to download all the binaries at once!

Your workflow seems specialized enough (running npm locally) that you need to manage the post-install issue yourself instead of preventing people from benefiting from this improvement. You can simply fork this package and create one package that has all the binaries included.

Because of the huge size of the package, I will not use lefthook in any of my public repositories that rely on the open-source contribution of people from around the world that have different connection speeds.

Looking at the number of dependants of lefthook and comparing it with Husky, shows the same thing. Only 2 packages depend on lefthook while 2000 depend on husky!

https://www.npmjs.com/package/@arkweid/lefthook
https://www.npmjs.com/package/husky

privatenumber · 2021-06-14T04:27:15Z

Seems @ringods's concern was neglected in #188:

I have customers in highly regulated environments where their working machines are isolated from the internet. Development is done with an internal package repository, acting as a proxy for upstream repositories. All package downloads come from this proxy.

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.

If the npm registry is proxied, I think this can be easily addressed by publishing the binaries to npm instead of using hosting them via GitHub release.

For example, playwright does this with their browser flavors (eg. playwright-chromium).

HellSquirrel reviewed Aug 7, 2019

View reviewed changes

Arkweid mentioned this pull request Nov 10, 2019

Fixed path to binaries #88

Merged

fleischie added 6 commits November 11, 2019 11:01

Download binary on npm postinstall hook

2a0deba

Try to fetch the binary files on postinstall for each supoorted OS and if not CI. This should reduce the npm package size (as you only ever download one binary for your host system).

Create binary directory if not existing, yet

e24d24c

Fallback to locally defined path

2ff89bb

If `process.env.INIT_CWD` is not set fall back to `resolve(".")` to try to emulate the behavior.

Handle Windows 32bit

17612d6

Exit early and notify user of issue, because currently there is only a windows 64bit version of lefthook.

Consolidate executable calling

cb0da0e

- Remove pre-build binaries in `bin/` folder, - rename downloaded executable to `lefthook` (or `lefthook.exe` on windows), and - call the appropriate executable by name (and platform) in `index.js`.

fleischie mentioned this pull request Nov 11, 2019

Speed up on Node #16

Closed

aminya mentioned this pull request May 15, 2021

Npm: download the required binaries during installation #188

Merged

Envek closed this in #188 Jun 2, 2021

Envek mentioned this pull request Jun 6, 2022

Split NPM package to two: bundled and installer #273

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Download binary on npm postinstall hook #43

Download binary on npm postinstall hook #43

fleischie commented Aug 2, 2019

Arkweid commented Aug 2, 2019

HellSquirrel commented Aug 2, 2019

HellSquirrel left a comment

HellSquirrel Aug 7, 2019

fleischie Aug 8, 2019

fleischie Aug 8, 2019

HellSquirrel Aug 10, 2019

levenkov Aug 13, 2019 •

edited

Loading

fleischie Aug 13, 2019

levenkov Aug 13, 2019

fleischie Aug 15, 2019

levenkov Aug 15, 2019

HellSquirrel commented Aug 11, 2019 •

edited

Loading

HellSquirrel commented Aug 11, 2019

fleischie commented Aug 13, 2019

ai commented Sep 26, 2019

fleischie commented Sep 26, 2019

HellSquirrel commented Sep 26, 2019

ai commented Sep 26, 2019

yumauri commented Oct 23, 2019

fleischie commented Oct 23, 2019

fleischie commented Oct 28, 2019 •

edited

Loading

fleischie commented Oct 28, 2019

yumauri commented Oct 28, 2019 •

edited

Loading

fleischie commented Nov 11, 2019

fleischie commented Nov 11, 2019

yumauri commented Nov 11, 2019

ai commented Jan 15, 2020 •

edited

Loading

ringods commented Feb 26, 2020

ai commented Feb 26, 2020

aminya commented May 10, 2021 •

edited

Loading

privatenumber commented Jun 14, 2021 •

edited

Loading

Download binary on npm postinstall hook #43

Download binary on npm postinstall hook #43

Conversation

fleischie commented Aug 2, 2019

Arkweid commented Aug 2, 2019

HellSquirrel commented Aug 2, 2019

HellSquirrel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

levenkov Aug 13, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HellSquirrel commented Aug 11, 2019 • edited Loading

HellSquirrel commented Aug 11, 2019

fleischie commented Aug 13, 2019

ai commented Sep 26, 2019

fleischie commented Sep 26, 2019

HellSquirrel commented Sep 26, 2019

ai commented Sep 26, 2019

yumauri commented Oct 23, 2019

fleischie commented Oct 23, 2019

fleischie commented Oct 28, 2019 • edited Loading

fleischie commented Oct 28, 2019

yumauri commented Oct 28, 2019 • edited Loading

fleischie commented Nov 11, 2019

fleischie commented Nov 11, 2019

yumauri commented Nov 11, 2019

ai commented Jan 15, 2020 • edited Loading

ringods commented Feb 26, 2020

ai commented Feb 26, 2020

aminya commented May 10, 2021 • edited Loading

privatenumber commented Jun 14, 2021 • edited Loading

levenkov Aug 13, 2019 •

edited

Loading

HellSquirrel commented Aug 11, 2019 •

edited

Loading

fleischie commented Oct 28, 2019 •

edited

Loading

yumauri commented Oct 28, 2019 •

edited

Loading

ai commented Jan 15, 2020 •

edited

Loading

aminya commented May 10, 2021 •

edited

Loading

privatenumber commented Jun 14, 2021 •

edited

Loading