8000 script: Let bootstrap install `apm` with `npm ci` by DeeDeeG · Pull Request #22450 · atom/atom · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content
This repository was archived by the owner on Mar 3, 2023. It is now read-only.

script: Let bootstrap install apm with npm ci #22450

Merged
merged 1 commit into from
May 20, 2021
Merged

script: Let bootstrap install apm with npm ci #22450

merged 1 commit into from
May 20, 2021

Conversation

DeeDeeG
Copy link
Contributor
@DeeDeeG DeeDeeG commented May 19, 2021
Requirements for Adding, Changing, or Removing a Feature (from template, click to expand):

Issue or RFC Endorsed by Atom's Maintainers

None, but takes advantage of a PR at apm that was approved by maintainers: atom/apm#912

Description of the Change

When bootstrapping/building Atom in ci mode, set NO_APM_DEDUPE="true" and install apm with npm ci (rather than npm install).

This enables faster, more-reproducible installs of apm, much like we already do for the scripts/ dependencies and the core Atom dependencies.

This only affects bootstrapping/building Atom; This should not make any difference to the built Atom app (except ensuring apm's dependencies are pinned to exact versions), and should not make any difference to apm's behavior as a command-line tool/as a component of Atom.

As of apm 2.6.2, apm respects a NO_APM_DEDUPE env var on Windows.
(It was already supported on Linux and macOS before then.)
When set, this env var disables the deduping
normally performed at the end of apm's postinstall script.

This deduping doesn't work properly when installing apm with npm ci,
for unknown reasons. (The deduping algorithm deletes many needed
dependencies, without reconstructing a valid tree.)

Now that apm supports NO_APM_DEDUPE on all platforms,
we can safely allow installing apm with npm ci
during the bootstrap script.

Now bootstrapping apm in ci mode is faster in two ways:

  • npm ci is generally faster than npm install for clean installs.
    • Great for actual automated builds ("CI").
  • The npm dedupe run is now skipped in ci mode.
    • The npm dedupe was of dubious value in any case
    • The npm dedupe command was also surprisingly slow

We also benefit from the stronger reproducibility of npm ci
compared to npm install (guaranteed, version-locked dependencies).

Alternate Designs

At the community fork of apm, we opted to delete deduping from apm's postinstall script altogether, so the NO_APM_DEDUPE env var isn't needed.

See: atom-community/apm#71

That can be done at the official apm repo too, if desired, but it could even be done after this PR. It need not block this PR.

Possible Drawbacks

None.

Verification Process

Ran script/bootstrap --ci and script/build --ci locally with this change.

Result: No broken/missing packages, and the "Installing apm" step of bootstrapping went a bit faster (because npm ci is faster than npm install for clean installs, and because it didn't spend any time deduping).

Ran this branch in Azure Pipelines.

Result: CI passed. https://dev.azure.com/DeeDeeG/b/_build/results?buildId=1127&view=results

Release Notes

N/A

@DeeDeeG
script: Let bootstrap install apm with npm ci
b042d2d 
This commit enables faster, more-reproducible installs of `apm`,
when bootstrapping/building Atom in `ci` mode.
(with `--ci` or env var `CI` set).

This only affects bootstrapping/building Atom; This should
not make any difference to the built Atom app, or to `apm`'s behavior
as a command-line tool/as a component of Atom.

Details:

As of apm 2.6.2, apm respects a `NO_APM_DEDUPE` env var on Windows.
(It was already supported on Linux and macOS before then.)
When set, this env var disables the deduping
normally performed at the end of apm's postinstall script.

This deduping doesn't work properly when installing apm with `npm ci`,
for unknown reasons. (The deduping algorithm deletes many needed
dependencies, without reconstructing a valid tree.)

Now that `apm` supports `NO_APM_DEDUPE` on all platforms,
we can safely allow installing `apm` with `npm ci`
during the bootstrap script.

Now bootstrapping apm in `ci` mode is faster in two ways:
- `npm ci` is generally faster than `npm install` for clean installs.
  - Great for actual automated builds ("CI").
- The `npm dedupe` run is now skipped in `ci` mode.
  - The `npm dedupe` was of dubious value in any case
  - The `npm dedupe` command was also surprisingly slow

We also benefit from the stronger reproducibility of `npm ci`
compared to `npm install` (guaranteed, version-locked dependencies).
sadick254
sadick254 approved these changes May 19, 2021
View reviewed changes
Copy link
Contributor
@sadick254 sadick254 left a comment

Choose a reason for hiding this comment

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

This looks good. I like the detailed description of the change. Could you open another PR on apm to remove deduping?

@DeeDeeG DeeDeeG mentioned this pull request May 19, 2021
Merged
@sadick254 sadick254 merged commit e72f05f into atom:master May 20, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@sadick254 sadick254 sadick254 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@DeeDeeG @sadick254
0