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

Jump to bottom

Update next-swc handling for PR stats #50933

Merged
merged 3 commits into from
Jun 7, 2023
Merged

Update next-swc handling for PR stats #50933

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b7e356d
Update next-swc handling for PR stats
ijjk Jun 7, 2023
7152ed9
Apply suggestions from code review
ijjk Jun 7, 2023
2c57de0
Merge branch 'canary' into fix/pr-stats-swc-loading
ijjk Jun 7, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
11 changes: 9 additions & 2 deletions .github/actions/next-stats-action/Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,20 @@
FROM node:16-bullseye
FROM ubuntu:22.04

LABEL com.github.actions.name="Next.js PR Stats"
LABEL com.github.actions.description="Compares stats of a PR with the main branch"
LABEL repository="https://github.com/vercel/next-stats-action"

COPY . /next-stats

RUN apt update && apt upgrade -y
RUN apt install unzip wget curl nano htop screen build-essential pkg-config libssl-dev git build-essential zlib1g-dev libncurses5-dev libgdbm-dev libnss3-dev libreadline-dev libffi-dev python3 moreutils jq iproute2 openssh-server sudo whois dnsutils -y

RUN ln $(which python3) /usr/bin/python

RUN curl -sfLS https://install-node.vercel.app/v18 | bash -s -- -f

# Install node_modules
RUN npm i -g pnpm@7.24.3
RUN npm i -g pnpm@7.24.3 yarn
RUN cd /next-stats && pnpm install --production
ijjk marked this conversation as resolved.
Show resolved Hide resolved

RUN git config --global user.email 'stats@localhost'
Expand Down
4 changes: 0 additions & 4 deletions .github/actions/next-stats-action/src/constants.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,6 @@ const mainRepoDir = path.join(workDir, 'main-repo')
const diffRepoDir = path.join(workDir, 'diff-repo')
const statsAppDir = path.join(workDir, 'stats-app')
const diffingDir = path.join(workDir, 'diff')
const yarnEnvValues = {
YARN_CACHE_FOLDER: path.join(workDir, 'yarn-cache'),
}
const allowedConfigLocations = [
'./',
'.stats-app',
Expand All @@ -25,6 +22,5 @@ module.exports = {
mainRepoDir,
diffRepoDir,
statsAppDir,
yarnEnvValues,
allowedConfigLocations,
}
3 changes: 3 additions & 0 deletions .github/actions/next-stats-action/src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,9 @@ if (!allowedActions.has(actionInfo.actionName) && !actionInfo.isRelease) {
)
.catch(console.error)

console.log(await exec(`ls ${path.join(__dirname, '../native')}`))
console.log(await exec(`cd ${dir} && ls ${dir}/packages/next-swc/native`))
Copy link
Member

Choose a reason for hiding this comment

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

You could probably pass a cwd option here instead of cd right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh cd isn't even needed here was just copied from other logs


logger(`Linking packages in ${dir}`)
const isMainRepo = dir === mainRepoDir
const pkgPaths = await linkPackages({
Expand Down
26 changes: 6 additions & 20 deletions .github/actions/next-stats-action/src/prepare/repo-setup.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@ module.exports = (actionInfo) => {
}
const pkgData = require(pkgDataPath)
const { name } = pkgData

pkgDatas.set(name, {
pkgDataPath,
pkg,
Expand All @@ -103,7 +104,7 @@ module.exports = (actionInfo) => {
if (!pkgData.files) {
pkgData.files = []
}
pkgData.files.push('native/*')
pkgData.files.push('native')
require('console').log(
'using swc binaries: ',
await exec(`ls ${path.join(path.dirname(pkgDataPath), 'native')}`)
Expand All @@ -124,18 +125,11 @@ module.exports = (actionInfo) => {
pkgData.dependencies['@next/swc'] =
pkgDatas.get('@next/swc').packedPkgPath
} else {
pkgData.files.push('native/*')
pkgData.files.push('native')
}
}
}

if (pkgData?.scripts?.prepublishOnly) {
// There's a bug in `pnpm pack` where it will run
// the prepublishOnly script and that will fail.
// See https://github.com/pnpm/pnpm/issues/2941
delete pkgData.scripts.prepublishOnly
}
Copy link
Member

Choose a reason for hiding this comment

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

I think we still need this since the linked issue was fixed in pnpm@7.26.0 but we are using pnpm@7.24.3 above

Copy link
Member Author

Choose a reason for hiding this comment

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

We're not using pnpm anymore, yarn pack just works so we should just keep using it 🤷

Copy link
Member

Choose a reason for hiding this comment

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

@ijjk This is only used for gh actions, it wont be used when I run pnpm test-start test/e2e/foo right?


await fs.writeFile(
pkgDataPath,
JSON.stringify(pkgData, null, 2),
Expand All @@ -147,21 +141,13 @@ module.exports = (actionInfo) => {
// to the correct versions
await Promise.all(
Array.from(pkgDatas.keys()).map(async (pkgName) => {
const { pkg, pkgPath, pkgData, packedPkgPath } = pkgDatas.get(pkgName)
// Copied from pnpm source: https://github.com/pnpm/pnpm/blob/5a5512f14c47f4778b8d2b6d957fb12c7ef40127/releasing/plugin-commands-publishing/src/pack.ts#L96
const tmpTarball = path.join(
pkgPath,
`${pkgData.name.replace('@', '').replace('/', '-')}-${
pkgData.version
}.tgz`
)
await execa('pnpm', ['pack'], {
const { pkgPath, packedPkgPath } = pkgDatas.get(pkgName)

await execa('yarn', ['pack', '-f', packedPkgPath], {
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer updating pnpm rather that switch back to yarn

Mixing package managers in the same repo can cause problems if you have corepack enable pnpm

Copy link
Member Author

Choose a reason for hiding this comment

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

This is only run in CI so isn't affected by corepack

cwd: pkgPath,
Copy link
Member

Choose a reason for hiding this comment

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

At the very least, add the COREPACK_ENABLE_STRICT=0 env var so that corepack wont error when running yarn

})
await fs.copyFile(tmpTarball, packedPkgPath)
})
)

return pkgPaths
},
}
Expand Down
25 changes: 4 additions & 21 deletions .github/actions/next-stats-action/src/run/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,12 @@
const path = require('path')
const fs = require('fs-extra')
const getPort = require('get-port')
const glob = require('../util/glob')
const exec = require('../util/exec')
const logger = require('../util/logger')
const getDirSize = require('./get-dir-size')
const collectStats = require('./collect-stats')
const collectDiffs = require('./collect-diffs')
const { statsAppDir, diffRepoDir, yarnEnvValues } = require('../constants')
const { statsAppDir, diffRepoDir } = require('../constants')

async function runConfigs(
configs = [],
Expand Down Expand Up @@ -59,13 +58,7 @@ async function runConfigs(

const buildStart = Date.now()
console.log(
await exec(
`cd ${statsAppDir} && ${statsConfig.appBuildCommand}`,
false,
{
env: yarnEnvValues,
}
)
await exec(`cd ${statsAppDir} && ${statsConfig.appBuildCommand}`, false)
)
curStats.General.buildDuration = Date.now() - buildStart

Expand Down Expand Up @@ -160,13 +153,7 @@ async function runConfigs(

const secondBuildStart = Date.now()
console.log(
await exec(
`cd ${statsAppDir} && ${statsConfig.appBuildCommand}`,
false,
{
env: yarnEnvValues,
}
)
await exec(`cd ${statsAppDir} && ${statsConfig.appBuildCommand}`, false)
)
curStats.General.buildDurationCached = Date.now() - secondBuildStart
}
Expand Down Expand Up @@ -203,13 +190,9 @@ async function linkPkgs(pkgDir = '', pkgPaths) {
}
await fs.writeFile(pkgJsonPath, JSON.stringify(pkgData, null, 2), 'utf8')

await fs.remove(yarnEnvValues.YARN_CACHE_FOLDER)
await exec(
`cd ${pkgDir} && pnpm install --strict-peer-dependencies=false`,
false,
{
env: yarnEnvValues,
}
false
)
}

Expand Down
1 change: 1 addition & 0 deletions .github/workflows/pull_request_stats.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ env:
NEXT_SKIP_NATIVE_POSTINSTALL: 1
TEST_TIMINGS_TOKEN: ${{ secrets.TEST_TIMINGS_TOKEN }}
NEXT_TEST_JOB: 1
NEXT_DISABLE_SWC_WASM: 1

jobs:
build:
Expand Down
13 changes: 8 additions & 5 deletions packages/next/src/build/swc/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,10 +95,13 @@ export async function loadBindings(): Promise<any> {
}

let attempts: any[] = []
const shouldLoadWasmFallbackFirst = triples.some(
(triple: any) =>
!!triple?.raw && knownDefaultWasmFallbackTriples.includes(triple.raw)
)
const disableWasmFallback = process.env.NEXT_DISABLE_SWC_WASM
const shouldLoadWasmFallbackFirst =
!disableWasmFallback &&
triples.some(
(triple: any) =>
!!triple?.raw && knownDefaultWasmFallbackTriples.includes(triple.raw)
)

if (shouldLoadWasmFallbackFirst) {
const fallbackBindings = await tryLoadWasmWithFallback(
Expand All @@ -117,7 +120,7 @@ export async function loadBindings(): Promise<any> {
}

// For these platforms we already tried to load wasm and failed, skip reattempt
if (!shouldLoadWasmFallbackFirst) {
if (!shouldLoadWasmFallbackFirst && !disableWasmFallback) {
const fallbackBindings = await tryLoadWasmWithFallback(
attempts,
isCustomTurbopack
Expand Down