Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Speed up npm ci by caching node_modules #45932

Merged
merged 8 commits into from
Mar 16, 2023
Merged

Speed up npm ci by caching node_modules #45932

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
f4ec7b3
Try custom setup-node composite action for aggressive caching
kevin940726 Nov 21, 2022
239050c
Add comment about GHA bug
kevin940726 Nov 21, 2022
2eff8c8
Try without rebuild and ignore-scripts
kevin940726 Dec 7, 2022
38bca97
Include npm version to the cache key
kevin940726 Dec 7, 2022
4014266
Try reverting the change of graceful-fs
kevin940726 Dec 7, 2022
65efe55
Update step name
kevin940726 Dec 7, 2022
1c9ca27
Code review
kevin940726 Mar 13, 2023
e437750
Add some comments
kevin940726 Mar 14, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 50 additions & 0 deletions .github/setup-node/action.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
name: 'Setup Node'
kevin940726 marked this conversation as resolved.
Show resolved Hide resolved
description: 'Setup node and install dependencies with caching support'
kevin940726 marked this conversation as resolved.
Show resolved Hide resolved
inputs:
node-version:
description: 'Optional node version to pass to setup-node'
kevin940726 marked this conversation as resolved.
Show resolved Hide resolved
required: false
type: string

runs:
using: 'composite'
steps:
- name: Use desired version of NodeJS from input
if: ${{ inputs.node-version != '' }}
uses: actions/setup-node@v3
with:
node-version: ${{ inputs.node-version }}
cache: npm

- name: Use desired version of NodeJS from .nvmrc
if: ${{ inputs.node-version == '' }}
uses: actions/setup-node@v3
with:
node-version-file: '.nvmrc'
kevin940726 marked this conversation as resolved.
Show resolved Hide resolved
cache: npm

- name: Get node and npm version
id: node-npm-version
run: |
echo "NODE_VERSION=$(node -v)" >> $GITHUB_OUTPUT
echo "NPM_VERSION=$(npm -v)" >> $GITHUB_OUTPUT
shell: bash

- name: Cache node_modules
id: cache-node_modules
uses: actions/cache@v3
kevin940726 marked this conversation as resolved.
Show resolved Hide resolved
with:
path: '**/node_modules'
key: node_modules-${{ runner.os }}-${{ steps.node-npm-version.outputs.NODE_VERSION }}/${{ steps.node-npm-version.outputs.NPM_VERSION }}-${{ hashFiles('package-lock.json') }}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm thinking that we don't need to include the npm version in the cache key here.

I can't think of a scenario where the version of npm would change independent of the Node.js version.

Copy link
Member

Choose a reason for hiding this comment

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

doesn't this happen regularly in Gutenberg since our npm version is still gated at <7?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think we need to include npm version here too. Including it will potentially bust the cache more often.

doesn't this happen regularly in Gutenberg since our npm version is still gated at <7?

That's because we're using Node 14, which comes with npm v6 by default. npm versions could change independently from Node.js, but they should just be minor releases and backward-compatible.

Copy link
Contributor

Choose a reason for hiding this comment

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

When a new version of Node.js is released, a specific version of npm is bundled with that version. You'll find an all inclusive manifest here. As long ass the npm install -g npm command is not run, then the version originally bundled with that version of Node.js will be the one installed.

I looked through the code of actions/setup-node. While I wasn't able to confirm this 100%, it seems that new version are pulled and built for use within Action runners using this manifest through the actions/node-versions and actions/versions-package-tools.

Because we only install Node 14.x, the version of npm will always be < 7, but the specific version of npm 6.x installed will be tied to the specific version of Node installed.

Copy link
Member

Choose a reason for hiding this comment

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

Right, but according to the project's package.json file we're good to use node@latest, which is I think what most people end up doing locally.

gutenberg/package.json

Lines 17 to 20 in 6434e0b

"engines": {
"node": ">=14.0.0",
"npm": ">=6.9.0 <7"
},

so maybe by happenstance we're only using node@14 in the test suites, but even then, are we guaranteed to run the specific version of 14? or are we pinning it entirely at a specific version?

Copy link
Member Author

Choose a reason for hiding this comment

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

so maybe by happenstance we're only using node@14 in the test suites

.nvmrc pins the Node.js version to 14 on CI. FWIW, there's an ongoing PR (#48950) that hopefully can be merged soon to upgrade Node.js and npm to latest on CI.

but even then, are we guaranteed to run the specific version of 14

Nope, if there's a minor/patch release then we'll automatically use the newer version on CI (basically ^14). But that's okay since we include the full node version in the cache key.

Note that this PR doesn't affect anyone's local setup whatsoever, they can continue using whatever supported Node.js/npm version they like locally. And the cache only exists on CI, so they won't be affected by it either. This PR only caches node_modules on CI, which is a controlled environment.

Copy link
Member

Choose a reason for hiding this comment

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

if there's a minor/patch release then we'll automatically use the newer version on CI (basically ^14). But that's okay since we include the full node version in the cache key.

I think we're in agreement here. my comment was specifically commenting on how we do expect changes to node to occur that don't match npm. we're intentionally not using the bundled version of npm in the project, but we build the project with changing versions of node quite often, since we pin node at >= a major version.


- name: Npm install
kevin940726 marked this conversation as resolved.
Show resolved Hide resolved
if: ${{ steps.cache-node_modules.outputs.cache-hit != 'true' }}
run: npm ci
shell: bash
kevin940726 marked this conversation as resolved.
Show resolved Hide resolved

- name: Post-install
kevin940726 marked this conversation as resolved.
Show resolved Hide resolved
if: ${{ steps.cache-node_modules.outputs.cache-hit == 'true' }}
run: |
npm run postinstall
npx lerna run postinstall
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the difference between running postinstall for npm and npx lerna? Do we need both?

If so, I'm wondering if we should split into two steps. That would make it more clear which of the two fails should either encounter a problem.

Copy link
Member Author

Choose a reason for hiding this comment

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

I added some comments explaining this ❤️ .

I don't think we need to split it into two steps though, as they have the same goal.

shell: bash
kevin940726 marked this conversation as resolved.
Show resolved Hide resolved
11 changes: 4 additions & 7 deletions .github/workflows/create-block.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,14 +26,11 @@ jobs:
steps:
- uses: actions/checkout@ac593985615ec2ede58e132d2e21d2b1cbd6127c # v3.3.0

- name: Use desired version of NodeJS
uses: actions/setup-node@64ed1c7eab4cce3362f8c340dee64e5eaeef8f7c # v3.6.0
- name: Setup Node and install dependencies
uses: ./.github/setup-node
with:
node-version: ${{ matrix.node }}
cache: npm

- name: npm install, build, format and lint
- name: Create block
shell: bash
run: |
npm ci
bash ./bin/test-create-block.sh
run: bash ./bin/test-create-block.sh
39 changes: 12 additions & 27 deletions .github/workflows/end2end-test.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,16 +29,11 @@ jobs:
steps:
- uses: actions/checkout@ac593985615ec2ede58e132d2e21d2b1cbd6127c # v3.3.0

- name: Use desired version of NodeJS
uses: actions/setup-node@64ed1c7eab4cce3362f8c340dee64e5eaeef8f7c # v3.6.0
with:
node-version-file: '.nvmrc'
cache: npm
- name: Setup Node and install dependencies
uses: ./.github/setup-node

- name: Npm install and build
run: |
npm ci
npm run build
- name: Npm build
Copy link
Contributor

Choose a reason for hiding this comment

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

We're running npm run build, but it's not just an npm build. Maybe "Run build scripts" would be more general and accurate. If we change this, it should get changed everywhere.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, let's do this in another PR though.

run: npm run build

- name: Install WordPress
run: |
Expand Down Expand Up @@ -78,16 +73,11 @@ jobs:
steps:
- uses: actions/checkout@ac593985615ec2ede58e132d2e21d2b1cbd6127c # v3.3.0

- name: Use desired version of NodeJS
uses: actions/setup-node@64ed1c7eab4cce3362f8c340dee64e5eaeef8f7c # v3.6.0
with:
node-version-file: '.nvmrc'
cache: npm
- name: Setup Node and install dependencies
uses: ./.github/setup-node

- name: Npm install and build
run: |
npm ci
npm run build
- name: Npm build
run: npm run build

- name: Install Playwright dependencies
run: |
Expand Down Expand Up @@ -137,19 +127,14 @@ jobs:
name: flaky-tests-report
path: flaky-tests

- name: Use desired version of NodeJS
- name: Setup Node and install dependencies
if: ${{ steps.download_artifact.outcome == 'success' }}
uses: actions/setup-node@64ed1c7eab4cce3362f8c340dee64e5eaeef8f7c # v3.6.0
with:
node-version-file: '.nvmrc'
cache: npm
uses: ./.github/setup-node

- name: Npm install and build
- name: Npm build
if: ${{ steps.download_artifact.outcome == 'success' }}
# TODO: We don't have to build the entire project, just the action itself.
run: |
npm ci
npm run build:packages
run: npm run build:packages

- name: Report flaky tests
if: ${{ steps.download_artifact.outcome == 'success' }}
Expand Down
11 changes: 2 additions & 9 deletions .github/workflows/performance.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,15 +23,8 @@ jobs:
steps:
- uses: actions/checkout@ac593985615ec2ede58e132d2e21d2b1cbd6127c # v3.3.0

- name: Use desired version of NodeJS
uses: actions/setup-node@64ed1c7eab4cce3362f8c340dee64e5eaeef8f7c # v3.6.0
with:
node-version-file: '.nvmrc'
cache: npm

- name: Npm install
run: |
npm ci
- name: Setup Node and install dependencies
uses: ./.github/setup-node

- name: Install specific versions of the themes used in tests
run: |
Expand Down
9 changes: 2 additions & 7 deletions .github/workflows/rnmobile-android-runner.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,13 +31,8 @@ jobs:
distribution: 'temurin'
java-version: '11'

- name: Use desired version of NodeJS
uses: actions/setup-node@64ed1c7eab4cce3362f8c340dee64e5eaeef8f7c # v3.6.0
with:
node-version-file: '.nvmrc'
cache: npm

- run: npm ci
- name: Setup Node and install dependencies
uses: ./.github/setup-node

- name: Gradle cache
uses: gradle/gradle-build-action@3fbe033aaae657f011f88f29be9e65ed26bd29ef # v2.3.3
Expand Down
9 changes: 2 additions & 7 deletions .github/workflows/rnmobile-ios-runner.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,13 +25,8 @@ jobs:
steps:
- uses: actions/checkout@ac593985615ec2ede58e132d2e21d2b1cbd6127c # v3.3.0

- name: Use desired version of NodeJS
uses: actions/setup-node@64ed1c7eab4cce3362f8c340dee64e5eaeef8f7c # v3.6.0
with:
node-version-file: '.nvmrc'
cache: npm

- run: npm ci
- name: Setup Node and install dependencies
uses: ./.github/setup-node

- name: Prepare build cache key
run: find package-lock.json packages/react-native-editor/ios packages/react-native-aztec/ios packages/react-native-bridge/ios -type f -print0 | sort -z | xargs -0 shasum | tee ios-checksums.txt
Expand Down
10 changes: 2 additions & 8 deletions .github/workflows/storybook-pages.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,14 +16,8 @@ jobs:
with:
ref: trunk

- name: Use desired version of NodeJS
uses: actions/setup-node@64ed1c7eab4cce3362f8c340dee64e5eaeef8f7c # v3.6.0
with:
node-version-file: '.nvmrc'
cache: npm

- name: Install Dependencies
run: npm ci
- name: Setup Node and install dependencies
uses: ./.github/setup-node

- name: Build Storybook
run: npm run storybook:build
Expand Down
37 changes: 12 additions & 25 deletions .github/workflows/unit-test.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,19 +33,16 @@ jobs:
steps:
- uses: actions/checkout@ac593985615ec2ede58e132d2e21d2b1cbd6127c # v3.3.0

- name: Use desired version of NodeJS
uses: actions/setup-node@64ed1c7eab4cce3362f8c340dee64e5eaeef8f7c # v3.6.0
- name: Setup Node and install dependencies
uses: ./.github/setup-node
with:
node-version: ${{ matrix.node }}
cache: npm

- name: Npm install and build
- name: Npm build
# It's not necessary to run the full build, since Jest can interpret
# source files with `babel-jest`. Some packages have their own custom
# build tasks, however. These must be run.
run: |
npm ci
npx lerna run build
run: npx lerna run build

- name: Running the tests
run: npm run test:unit -- --ci --maxWorkers=2 --cacheDirectory="$HOME/.jest-cache"
Expand Down Expand Up @@ -79,11 +76,8 @@ jobs:
steps:
- uses: actions/checkout@ac593985615ec2ede58e132d2e21d2b1cbd6127c # v3.3.0

- name: Set up Node.js
uses: actions/setup-node@64ed1c7eab4cce3362f8c340dee64e5eaeef8f7c # v3.6.0
with:
node-version-file: '.nvmrc'
cache: npm
- name: Setup Node and install dependencies
uses: ./.github/setup-node

##
# This allows Composer dependencies to be installed using a single step.
Expand Down Expand Up @@ -116,10 +110,8 @@ jobs:
with:
custom-cache-suffix: $(/bin/date -u --date='last Mon' "+%F")

- name: Install npm dependencies
run: |
npm ci
npm run build
- name: Npm build
desrosj marked this conversation as resolved.
Show resolved Hide resolved
run: npm run build

- name: Docker debug information
run: |
Expand Down Expand Up @@ -235,19 +227,14 @@ jobs:
steps:
- uses: actions/checkout@ac593985615ec2ede58e132d2e21d2b1cbd6127c # v3.3.0

- name: Use desired version of NodeJS
uses: actions/setup-node@64ed1c7eab4cce3362f8c340dee64e5eaeef8f7c # v3.6.0
with:
node-version-file: '.nvmrc'
cache: npm
- name: Setup Node and install dependencies
uses: ./.github/setup-node

- name: Npm install and build
- name: Npm build
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, this seems to be running the Lerna build script.

Suggested change
- name: Npm build
- name: Build Lerna

Copy link
Member Author

@kevin940726 kevin940726 Mar 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

"Build Lerna" isn't really descriptive either though. I think this should be "Build packages" and just run npm run build:packages. But it's already the way it is in trunk, so maybe we should change it in another PR?

# It's not necessary to run the full build, since Jest can interpret
# source files with `babel-jest`. Some packages have their own custom
# build tasks, however. These must be run.
run: |
npm ci
npx lerna run build
run: npx lerna run build

- name: Running the tests
run: npm run native test -- --ci --maxWorkers=2 --cacheDirectory="$HOME/.jest-cache"