[BUG] "npm rebuild" with v7 runs the postinstall script, whereas it did not run the postinstall script with npm v6 (can cause unexpected endless loop)

[DeeDeeG]

Current Behavior:

Putting npm rebuild in a postinstall script can cause an infinite loop with npm v7.

Expected Behavior:

npm rebuild used to not trigger the postinstall script (so, no infinite loop) with npm v6.

Steps To Reproduce:

1. Make a new blank project: mkdir x && cd x && npm init -y
2. Put this script in your package.json: "postinstall": "npm rebuild"
3. Run npm install or npm rebuild
   i. npm install --verbose shows what's going on clearer.
4. Observe: npm install runs postinstall which runs rebuild which triggers postinstall which runs rebuild... ♾️
5. (Optional): Open a task manager or system monitor to see that many copies of npm are spawned recursively.

Environment:

• OS: Tested on: macOS 10.15.7, Windows 10, Ubuntu 20.04 (and happens to some lesser extent on Windows Subsystem for Linux)
• Node: Tested on: 12.20
• npm: 7.1.1 and newer are affected

[DeeDeeG]

Technically, this package.json is the minimal reproducer:

{
  "scripts": {
    "postinstall": "npm rebuild"
  }
}

(And then run npm install, optionally with --verbose).

But you could have a more complex postinstall script that does something useful and then runs npm rebuild.

Real world example: The package manager for the Atom text editor does this and is infinitely looping with npm 7:
https://github.com/atom/apm/blob/v2.6.1/script/postinstall.sh#L9-L10

(It bundles a specific copy of Node to run itself with, and so during postinstall it needs its own native dependencies to be rebuilt with its bundled Node, as opposed to built against whatever different version of Node the user might have on their PATH.)

[aminya]

I can reproduce this issue. Seems like a regression in npm 7, which results in infinite loops when you use rebuild inside your postinstall.

[DeeDeeG]

This bug doesn't happen with npm <= v7.1.0, but it does happen with v7.1.1 and higher.

7.1.1+ are affected.

[darcyclarke]

@DeeDeeG as a quick question, wondering why you need to be running npm rebuild after you install? Since install should build your deps (ie. compile bins, run lifecycle scripts etc.). I don't think this is a problem with npm itself. I think a best practice here is to not run rebuild in a postinstall script. We've got a separate issue going to better document lifecycle scripts as well (which I think should help in the future): npm/statusboard#267

[DeeDeeG]

Hi @darcyclarke. I considered writing out the use case in detail above but didn't want to be too verbose. Here's the details:

• Atom's package manager (apm) bundles a version of Node. This is the version of Node that apm runs itself with in production. apm is a core component of the Atom text editor. It is coded to run as a standalone CLI app, not to be required and run as JS. So it is coded as a module shipped with its own runtime (its bundled Node binary).
• During development, there needs to be a way to build apm's own node_modules dependencies against the same (bundled) version of Node it will run itself with in production.
  • (Ideally apm's dependencies should be built with the very same binary of Node that it will be bundled with, to avoid hypothetical non-standard Node builds with unforeseen build flags, or whatever the case may be.)
• In order for apm to be convenient to develop, regardless of the Node version on the user's system, the postinstall script downloads a binary of a fixed version of Node, and rebuilds apm's dependencies with the bundled Node binary. (This rebuilding isn't much of a leap beyond downloading a Node binary, as we already need to do for bundling purposes).

We (apm maintainers) have a couple of options here to workaround the behavior change that started in npm v7.1.1, I suppose:

Potential workaround for apm (click to expand):

• In our postinstall script, set an env var APM_ALREADY_REBUILDING just before doing npm rebuild, and quit the postinstall script early if that's set. Breaks the infinite loop.
• We could finish research into running apm directly with Electron, (atom/apm@230a25f, https://www.electronjs.org/docs/api/environment-variables#electron_run_as_node) but it's not clear yet if that's viable (more research needed). That would mean we wouldn't need to bundle a Node binary with apm. (This still might be tricky to do without an npm rebuild step.)
• We could change the build process so that the bundled Node is downloaded first thing, and the whole npm install and postinstall scripts and so on could be run with that version of Node. No rebuilding necessary. (This is tricky, because we're very limited in what we can do using Node/npm scripts to get the bundled version of Node, as some of this would apparently need to happen before doing an npm install to get our devDependencies.)
  • On a similar note, we could pass the target Node version to npm --> node-gyp so it knows to build for the same Node version we're bundling. This might be easier and might not even need any devDependencies. https://github.com/nodejs/node-gyp#command-options

More context and my thoughts:

Workarounds for apm aside, @aminya has told me that running npm rebuild in postinstall scripts is quite common. So if that's the case, this is going to cause a lot of folks to need to rejigger their postinstall scripts to workaround this change in behavior.

I think there are many situations where you'd just want to rebuild native code, rather than "rebuild native code and also run your postinstall scripts". And I can't foresee as many situations where "rebuild and run postinstall scripts" is more desirable.

Having it just rebuild is much more composable for scripting. Having it do both means some rather involved or awkward workarounds might be required, as in apm's case. I personally think, from a package author/maintainer's perspective, having npm rebuild not run postinstall scripts is a much better developer experience. (But as long as the behavior is predictable, we can find a way to work around it if the default behavior doesn't meet our needs.)

We've got a separate issue going to better document lifecycle scripts as well (which I think should help in the future) npm/statusboard#267

👍 Whatever the outcome that gets decided, more documentation would be great!

P.S. for the record I'm not an official member of the Atom team. I'm a regular contributor from the comunity. And a lot of the core work of that text editor is actually being done by community members now that the main team is pretty lean on personnel and work hours devoted to Atom.

[DeeDeeG]

@DeeDeeG as a quick question, wondering why you need to be running npm rebuild after you install?

The short version is:

• apm authors once upon a time decided to have apm bundle a version of Node and have apm run itself with that bundled Node runtime in production. (This makes apm a self-contained runnable-on-its-own CLI app. Nice.)
• Developers contributing to apm may not have the same version of Node on their PATH as the bundled version, and in any case building dependencies with the real bundled Node binary itself is desirable.
• Building apm's dependencies programatically with a locally downloaded Node binary, anywhere other than in a postinstall script, is not super fun because we have no devDependencies until after we've done npm install. Doing this in the postinstall script was a really natural and obvious choice in the pre-npm-v7 days.
  • Now that's causing infinite loops.

Oh, and the main Atom repo treats apm as a regular Node module. It's installed with npm install. So any solution that is totally outside of the npm install lifecycle is a no-go at the moment. (We could theoretically rewrite both apm's and Atom's build scripts to workaround this, but it would be great not to need to do that.)

[ljharb]

npm install already builds tho, so running npm rebuild in postinstall seems redundant?

[DeeDeeG]

I have a bad habit of saying things with way too long posts.

We find it extremely useful to make sure "the version of Node running npm install" is not the version of Node our dependencies build for.

Edit to add: (And we really want to make sure this is handled by apm's package.json postinstall script, so that we can do npm install --global-style atom-package-manager in our larger Atom repo. Simply setting npm_config_target when running that command might be doable, but would be more awkward and the configuration for that would spill out to the main Atom repo, meaning more cross-repo coordination when making changes to the build scripts in either repo. And then forgetting to set that env var breaks apm.)

If there are more specific follow-up questions about this use-case, I'd be glad to answer. (And I'll try to keep the answers concise and to the point.) Thanks for your time.

[ljharb]

Right, i totally get that - but npm rebuild inside a postinstall script is going to be for the same version of node that did the install, no?

Running npm rebuild on its own, from another node version, seems like it would achieve what you want.

[DeeDeeG]

We have a project-local shim for npm that uses the local copy of Node to run the local copy of npm. Our postinstall script calls the shim with [shim-npm-here] rebuild.

Running npm rebuild on its own, from another node version, seems like it would achieve what you want.

That's what we're doing and where we're seeing the endless loops. The top post of this issue has a farcical but minimal reproducer. It is what our real use case boils down to, in some sense. (Minus the real-world useful stuff that we do.)

This used to make sense to do in a postinstall script. But now it loops.

(If we manually "ran npm on its own with another node version", given this bug (IMO it's a bug), rebuilding would hit the postinstall script. The only way to not loop is to kill the convenience of doing it in postinstall. Or some slightly ugly workarounds.)

[DeeDeeG]

I acknowledge that our use-case us unusual, but I personally maintain that npm rebuild running the postinstall script is less composable for script authors.

This is less about whether running npm rebuild in a postinstall makes sense, and more about whether users should expect their postinstall scripts to run again if doing npm rebuild, at which point, logically speaking, the postinstall script has probably already been run once.

A lot of postinstall scripts aren't meant to run more than once. And npm rebuild is pretty much only used to make the native code "fresh". I don't usually think "and then I want to run my postinstall script" when doing that.

What is the use-case for npm rebuild where running postinstall afterward is desirable? (I don't mean to be confrontational, just I don't think this meaningful behavior change in npm is a step in a better direction vs v6.)

[aminya]

I think a best practice here is to not run rebuild in a postinstall script.

You can't assume that people don't do that. Automatically, running postinstall after rebuild is in my opinion completely unnecessary, and it is a new behavior that people are not used to. If someone wants to run both postinstall and rebuild, they will use install instead. But when someone runs rebuild, they really only want to rebuild not anything else.

If this is the desired behavior you should bump the npm version to v8 because it is a breaking change for v7 (and older versions). Currently, npm <= v7.1.0 works fine, and the newer versions are faulty.

Please open the issue. The severity of this bug is more than the documentation.

[DeeDeeG]

In my opinion postinstall scripts are for after a normal complete install. They do things that make a package ready for use, and they're not expected to run often.

Times when I'm rebuilding the native C++ code are rarely (pretty much never) times when the project is in a state ready for postinstall to run. And if I were to want to run postinstall after doing rebuild I'd strongly expect that to be opt-in, something I have to do explicitly.

[darcyclarke]

@DeeDeeG / @aminya appreciate the extra context, again we've got another issue/spike in the works that should further lement lifecycle scripts; I believe there's been some confusion & 100% a lack of concrete documentation in this space which isn't good. We can circle back on this early next week but I should note that we will probably further document/outline how npm@7 functions vs. trying to match or document the behavior of v6 more clearly

[DeeDeeG]

@darcyclarke @ljharb thanks for continuing to engage here.

I hope it's not too redundant to this issue, but I posted this as a "feature sugestion" over here: npm/feedback#218. I want to be able to voice an opinion about what the intended behavior should be in npm 7, and potentially let there be a space for community discussion and comments, and it's seems like perhaps that's the right way to do that rather than a bug report.

Best regards.

[DeeDeeG]

The problem is wider than I initially realized: npm rebuild now runs all three of: the preinstall, install and postinstall scripts. So whereas maybe one of those is a better fit than postinstall, now none of those options are viable as they all infinite loop.

[DeeDeeG]

[...] I should note that we will probably further document/outline how npm@7 functions vs. [doing it the npm v6 way]

Okay. I've fully made the case as well as I can make it, above.

I would like to suggest a couple of things, assuming the current npm rebuild v7 behavior is formalized/documented:

• Consider making this configurable, ideally with a flag. Such as --no-lifecycle-scripts. Or have npm rebuild --ignore-scripts not disable the rebuilding functionality.
• Looking forward to any future npm v8 release, please do consider reverting to the npm v6 behavior, and/or collectiong some user research into which is the ideal behavior. (I concur with @aminya on this.)
• Please consider emphasizing in the docs that this is a change from v6, and advise users to make their old scripts "idempotent" -- that is, make sure their preinstall/install/postinstall scripts "do no harm" when run multiple times -- if intending to run npm rebuild. (Particularly, script authors should make these scripts capable of avoiding infinite loops by not calling npm rebuild recursively from another npm rebuild. (Example: DeeDeeG/apm@eaed703)). I would be willing to work on a docs PR to find an appropriate way to highlight this information.