atom / apm

Atom Package Manager
https://atom.io/packages
MIT License
1.27k stars 294 forks source link

Respect the "NO_APM_DEDUPE" env var on Windows (for the postinstall script) #912

Closed DeeDeeG closed 3 years ago

DeeDeeG commented 3 years ago

Requirements

Description of the Change

Linux and macOS have respected a NO_APM_DEDUPE env var for a long time. When this env var is set, the postinstall script skips running npm dedupe.

This PR adds a few lines to the Windows-specific script\postinstall.cmd file to do the same thing on Windows.

Alternate Designs

Benefits

This enables installing apm with npm ci in the Atom repository.

Which in turn means we can have reliable, stable bootstrapping, when desired, for the Atom repo. No unexpected dependency changes, and no more hard-to-troubleshoot and hard-to-fix build failures.

(Note: Attempting to "disable deduping" may not work when installing apm as a git URL dependency. But installing apm as a git URL depoendency has already been buggy for a long time, regardless of this PR. And this PR would not be a regression, just that the improvement here may not apply to apm as a git URL dependency.)

Possible Drawbacks

None.

Verification Process

For this repository on its own:

I ran npm run postinstall and npm install with this env var set (set NO_APM_DEDUPE=true in cmd.exe).

Deduping was successfully skipped, and a message was printed out (>> Deduplication disabled) to indicate that this is happening on purpose.


For testing out installing apm in Atom with npm ci:

Applicable Issues

None.

DeeDeeG commented 3 years ago

@aminya and I are strongly considering turning off deduping to some extent in the community fork of apm. I think that would be a good idea here, too. Even if it's just "off by default," but enableable with a flag.

Removing deduping entirely from the postinstall script has crossed my mind many times when installing apm.

The downside of running npm dedupe is that it sometimes leaves you with missing packages. And running it in the postinstall script is incompatible with npm ci.

There is often no "upside" to running npm dedupe, as the lockfile already records an optimally deduped dependency tree. And npm install makes a good effort to dedupe; In theory, it will tend to fully dedupe the tree. (In practice I think it sometimes holds back just a bit).

The only time npm dedupe should be run, in my opinion, is over in the Atom repository, when upgrading the apm bundled with Atom. And even then, I think it should be done manually and verified by running npm ls to see if there are any missing packages. After which point, subsequent installs will look at the optimized tree in the lockfile, and there will be no need for further deduping. This would greatly enhance the stability of installs in the apm dir of Atom over time.

aminya commented 3 years ago

If I was the one who makes the decision, I would say let's remove it. But because this is going to be upstreamed, we can go with this PR to be consistent with linux/mac.

In a separate PR let's remove this deduping altogether.