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
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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
approved these changes
May 19, 2021
Contributor
sadick254
left a comment
sadick254
left a comment
There was a problem hiding this comment.
This looks good. I like the detailed description of the change. Could you open another PR on apm to remove deduping?
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
apmthat was approved by maintainers: atom/apm#912Description of the Change
When bootstrapping/building Atom in
cimode, setNO_APM_DEDUPE="true"and installapmwithnpm ci(rather thannpm install).This enables faster, more-reproducible installs of
apm, much like we already do for thescripts/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 toapm's behavior as a command-line tool/as a component of Atom.As of apm 2.6.2, apm respects a
NO_APM_DEDUPEenv 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
apmsupportsNO_APM_DEDUPEon all platforms,we can safely allow installing
apmwithnpm ciduring the bootstrap script.
Now bootstrapping apm in
cimode is faster in two ways:npm ciis generally faster thannpm installfor clean installs.npm deduperun is now skipped incimode.npm dedupewas of dubious value in any casenpm dedupecommand was also surprisingly slowWe also benefit from the stronger reproducibility of
npm cicompared to
npm install(guaranteed, version-locked dependencies).Alternate Designs
At the community fork of
apm, we opted to delete deduping fromapm'spostinstallscript altogether, so theNO_APM_DEDUPEenv var isn't needed.See: atom-community/apm#71
That can be done at the official
apmrepo 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 --ciandscript/build --cilocally with this change.Result: No broken/missing packages, and the "Installing apm" step of bootstrapping went a bit faster (because
npm ciis faster thannpm installfor 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