Skip to content
This repository has been archived by the owner on Dec 15, 2022. It is now read-only.

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

Merged

Conversation

DeeDeeG
Copy link
Contributor

@DeeDeeG DeeDeeG commented Jan 13, 2021

Requirements

  • Filling out the template is required. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.
  • All new code requires tests to ensure against regressions

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

  • Make deduping off by default (actually, I think I prefer this over "deduping is on by default", now that I'm thinking about it.)
  • Remove deduping entirely from the postinstall script. (This would also be a good option).

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:

  • cd atom\apm
  • Set NO_APM_DEDUPE=true
  • npm ci
  • While this PR is pending, testing this feature with Atom requires patching apm in-place to use the updated script\postinstall.cmd.
    • During npm ci, quickly after apm is extracted, copy the modified script\postinstall.cmd and paste to overwrite the existing, non-patched one in atom\apm\node_modules\atom-package-manager\script\postinstall.cmd.
    • (Live-patching this file on Windows will no-longer be needed once this PR is merged and shipped in a stable release of apm.)
    • If you are on Linux or macOS, then there is no need to copy or patch any files; The flag is already respected on Linux/macOS.
  • Observe that the following is printed at the end: >> Deduplication disabled
  • Run npm ls, and observe that no packages are reported missing.

Applicable Issues

None.

This environment var is already used for this purpose on Linux/macOS.
We should respect this env var on Windows, too.
@DeeDeeG
Copy link
Contributor Author

DeeDeeG commented Jan 13, 2021

@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
Copy link
Contributor

aminya commented Jan 13, 2021

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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants