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.

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

Reduce install size for linux glibc/musl #32850

Merged
merged 5 commits into from
Jan 3, 2022
Merged

Conversation

styfle
Copy link
Member

@styfle styfle commented Dec 27, 2021

In Next.js 12.0.1, musl support was added which caused linux to install both glibc and musl binaries.

This PR adds the install script to prevent installing unused binaries, reducing the install size by 47MB.

We originally thought this could be added to Node.js core and thus npm but it was rejected.

Note getReport() works on Node.js >=11.8.0 which is safe to use since Next.js requires "node": ">=12.22.0".

@ijjk

This comment has been minimized.

Copy link
Member

@ijjk ijjk left a comment

Choose a reason for hiding this comment

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

Does this still require downloading all of the packages since they are still matching optional dependencies and the install script can't run until downloaded? If so could we instead move this as an install script on next package itself and only download and unpack the matching swc package for the linux system?

We could potentially expand on this script in a future PR to fallback to downloading the wasm variant if we don't have a binary for the specific platform yet since we can't currently have a fallback package with optional dependencies.

@Brooooooklyn
Copy link
Contributor

Skip incompatible libc packages via postinstall scripts is still risky because npm/rfcs#488

Can we continue to communicate with the Node.js team to resolve this issue? I personally dislike the postinstall scripts solution.

@styfle
Copy link
Member Author

styfle commented Dec 28, 2021

If so could we instead move this as an install script on next package itself and only download and unpack the matching swc package for the linux system?

@ijjk Are you thinking of removing the optionalDependencies and performing the install via node-fetch or something like that?

@Brooooooklyn I think its good to pursue a canonical solution in npm/rfcs#488 in the long term, but we need a solution for the short term too. (any change to npm install would rely on users to upgrade npm and also for other package managers to implement the same feature)

@Brooooooklyn
Copy link
Contributor

From nodejs/node#41338 (comment), we may not need to introduce detect-libc.

// postinstall.js in `linux-[x64|aarch64]-[gnu|musl]` packages

const { glibcVersionRuntime } = process.report.getReport().header

if (glibcVersionRuntime) {
  // Glibc
} else {
  // musl libc
}

@ijjk
Copy link
Member

ijjk commented Dec 29, 2021

@styfle yeah exactly, at least removing the linux variants that cause the duplicate downloads from optionalDependencies we can leave the others and then we can download the correct package from npm using node-fetch and only unpack that one in next.

the above process.report solution looks like a good alternative to the detect-libc package, if we only want to use this script to install the libc/musl swc packages we probably want to wrap the script in a if (process.report.getReport().header.osName === 'Linux') check.

@Brooooooklyn
Copy link
Contributor

we probably want to wrap the script in a if (process.report.getReport().header.osName === 'Linux') check.

We even don't need the os check, because we can use optionalDependencies + postinstall check. The scripts will only be executed on Linux.

@ijjk
Copy link
Member

ijjk commented Dec 29, 2021

Doesn't using optionalDependencies require downloading the additional binaries still for the script to run or is this more optimal as it can then be cached by the package manager?

@ijjk

This comment has been minimized.

@Brooooooklyn
Copy link
Contributor

is this more optimal as it can then be cached by the package manager

Yes, if we download binary via custom scripts like node-fetch, we'll need to maintain the cache logic ourselves. Or the users will need to re-download next-swc whenever they start a new next project.

@yisibl
Copy link

yisibl commented Dec 30, 2021

Maybe this solution can be added to the napi-rs cli.

@styfle
Copy link
Member Author

styfle commented Jan 3, 2022

I agree that a custom script at the top level is a little more risky.

This PR solves the issue of the user doing npm install --ignore-scripts but if we used the install script at the root package, then that would fail because our custom script would be necessary to install swc.

We would also need to solve the issue of where to install swc since yarn 2 and pnpm use a different algorithm than npm so we might not be able to just create node_modules/@next/swc-foo with a custom script. Its best to let the package manager do it.

This PR is the least risky thing we can do today because the best case scenario is disk space is reduced and worst case is that it behaves the same and both glibc and musl variants are installed.

@styfle styfle requested a review from ijjk January 3, 2022 20:14
@styfle styfle requested a review from ijjk January 3, 2022 20:33
@ijjk
Copy link
Member

ijjk commented Jan 3, 2022

Stats from current PR

Default Build (Decrease detected ✓)
General Overall increase ⚠️
vercel/next.js canary vercel/next.js reduce-swc-size-glibc-musl Change
buildDuration 13.7s 13.6s -71ms
buildDurationCached 3.2s 3.1s -91ms
nodeModulesSize 348 MB 348 MB ⚠️ +9 B
Page Load Tests Overall decrease ⚠️
vercel/next.js canary vercel/next.js reduce-swc-size-glibc-musl Change
/ failed reqs 0 0
/ total time (seconds) 2.724 2.733 ⚠️ +0.01
/ avg req/sec 917.68 914.68 ⚠️ -3
/error-in-render failed reqs 0 0
/error-in-render total time (seconds) 1.153 1.155 0
/error-in-render avg req/sec 2167.72 2165 ⚠️ -2.72
Client Bundles (main, webpack, commons)
vercel/next.js canary vercel/next.js reduce-swc-size-glibc-musl Change
450.HASH.js gzip 179 B 179 B
framework-HASH.js gzip 42.2 kB 42.2 kB
main-HASH.js gzip 27.1 kB 27.1 kB
webpack-HASH.js gzip 1.45 kB 1.45 kB
Overall change 70.9 kB 70.9 kB
Legacy Client Bundles (polyfills)
vercel/next.js canary vercel/next.js reduce-swc-size-glibc-musl Change
polyfills-HASH.js gzip 31 kB 31 kB
Overall change 31 kB 31 kB
Client Pages
vercel/next.js canary vercel/next.js reduce-swc-size-glibc-musl Change
_app-HASH.js gzip 1.37 kB 1.37 kB
_error-HASH.js gzip 194 B 194 B
amp-HASH.js gzip 312 B 312 B
css-HASH.js gzip 326 B 326 B
dynamic-HASH.js gzip 2.37 kB 2.37 kB
head-HASH.js gzip 350 B 350 B
hooks-HASH.js gzip 919 B 919 B
image-HASH.js gzip 4.74 kB 4.74 kB
index-HASH.js gzip 263 B 263 B
link-HASH.js gzip 2.13 kB 2.13 kB
routerDirect..HASH.js gzip 321 B 321 B
script-HASH.js gzip 383 B 383 B
withRouter-HASH.js gzip 318 B 318 B
85e02e95b279..7e3.css gzip 107 B 107 B
Overall change 14.1 kB 14.1 kB
Client Build Manifests
vercel/next.js canary vercel/next.js reduce-swc-size-glibc-musl Change
_buildManifest.js gzip 459 B 459 B
Overall change 459 B 459 B
Rendered Page Sizes
vercel/next.js canary vercel/next.js reduce-swc-size-glibc-musl Change
index.html gzip 533 B 533 B
link.html gzip 547 B 547 B
withRouter.html gzip 527 B 527 B
Overall change 1.61 kB 1.61 kB

Default Build with SWC (Decrease detected ✓)
General Overall increase ⚠️
vercel/next.js canary vercel/next.js reduce-swc-size-glibc-musl Change
buildDuration 15.1s 15.1s ⚠️ +6ms
buildDurationCached 3.1s 3.1s ⚠️ +27ms
nodeModulesSize 348 MB 348 MB ⚠️ +9 B
Page Load Tests Overall decrease ⚠️
vercel/next.js canary vercel/next.js reduce-swc-size-glibc-musl Change
/ failed reqs 0 0
/ total time (seconds) 2.716 2.719 0
/ avg req/sec 920.39 919.55 ⚠️ -0.84
/error-in-render failed reqs 0 0
/error-in-render total time (seconds) 1.128 1.139 ⚠️ +0.01
/error-in-render avg req/sec 2217.22 2194.22 ⚠️ -23
Client Bundles (main, webpack, commons)
vercel/next.js canary vercel/next.js reduce-swc-size-glibc-musl Change
450.HASH.js gzip 179 B 179 B
framework-HASH.js gzip 42.3 kB 42.3 kB
main-HASH.js gzip 27.2 kB 27.2 kB
webpack-HASH.js gzip 1.44 kB 1.44 kB
Overall change 71.1 kB 71.1 kB
Legacy Client Bundles (polyfills)
vercel/next.js canary vercel/next.js reduce-swc-size-glibc-musl Change
polyfills-HASH.js gzip 31 kB 31 kB
Overall change 31 kB 31 kB
Client Pages
vercel/next.js canary vercel/next.js reduce-swc-size-glibc-musl Change
_app-HASH.js gzip 1.35 kB 1.35 kB
_error-HASH.js gzip 180 B 180 B
amp-HASH.js gzip 305 B 305 B
css-HASH.js gzip 321 B 321 B
dynamic-HASH.js gzip 2.36 kB 2.36 kB
head-HASH.js gzip 342 B 342 B
hooks-HASH.js gzip 906 B 906 B
image-HASH.js gzip 4.76 kB 4.76 kB
index-HASH.js gzip 256 B 256 B
link-HASH.js gzip 2.19 kB 2.19 kB
routerDirect..HASH.js gzip 314 B 314 B
script-HASH.js gzip 375 B 375 B
withRouter-HASH.js gzip 309 B 309 B
85e02e95b279..7e3.css gzip 107 B 107 B
Overall change 14.1 kB 14.1 kB
Client Build Manifests
vercel/next.js canary vercel/next.js reduce-swc-size-glibc-musl Change
_buildManifest.js gzip 459 B 459 B
Overall change 459 B 459 B
Rendered Page Sizes
vercel/next.js canary vercel/next.js reduce-swc-size-glibc-musl Change
index.html gzip 531 B 531 B
link.html gzip 545 B 545 B
withRouter.html gzip 526 B 526 B
Overall change 1.6 kB 1.6 kB
Commit: e271a63

@kodiakhq kodiakhq bot merged commit a5fab84 into canary Jan 3, 2022
@kodiakhq kodiakhq bot deleted the reduce-swc-size-glibc-musl branch January 3, 2022 21:17
@jakejarvis
Copy link
Contributor

jakejarvis commented Jan 3, 2022

@styfle Just bumped to canary.15 and started getting this error in the logs when deploying on Vercel (but it still builds and deploys successfully — I have swcMinify: true if that matters). Let me know if more info would be helpful. :)

18:08:21.131 | Installing dependencies...
18:08:21.440 | yarn install v1.22.17
18:08:21.559 | [1/4] Resolving packages...
18:08:22.033 | [2/4] Fetching packages...
18:08:54.984 | [3/4] Linking dependencies...
18:08:54.990 | warning "styled-jsx > @babel/plugin-syntax-jsx@7.14.5" has unmet peer dependency "@babel/core@^7.0.0-0".
18:09:06.360 | [4/4] Building fresh packages...
18:09:06.828 | warning Error running install script for optional dependency: "/vercel/path0/node_modules/@next/swc-linux-x64-musl: Command failed.
18:09:06.828 | Exit code: 1
18:09:06.829 | Command: node install.js
18:09:06.829 | Arguments:
18:09:06.832 | Directory: /vercel/path0/node_modules/@next/swc-linux-x64-musl
18:09:06.832 | Output:
18:09:06.833 | "
18:09:06.838 | info This module is OPTIONAL, you can safely ignore this error
18:09:08.143 | Done in 46.71s.
18:09:08.182 | Detected Next.js version: 12.0.8-canary.15

edit: I believe my logs should be public... https://vercel.com/jakejarvis/jarvis/ctoRCH1hj3SLViXxqWRQjqstUkY4

@merceyz
Copy link
Contributor

merceyz commented Jan 3, 2022

I struggle to see how this reduces the install size, by the time the script runs the files are already written to disk

ijjk added a commit that referenced this pull request Jan 4, 2022
@styfle
Copy link
Member Author

styfle commented Jan 4, 2022

I struggle to see how this reduces the install size, by the time the script runs the files are already written to disk

The title is a bit of a misnomer: this PR doesn't reduce network bandwidth, it only reduces disk space since npm will delete a dependency if the install script fails.

That being said. It seems like yarn 1.x is really noisy when this script is working properly so its probably best to revert and try getting this feature upstream.

ijjk added a commit that referenced this pull request Jan 4, 2022
This reverts commit a5fab84.

Co-authored-by: JJ Kasper <jj@jjsweb.site>
teleaziz added a commit to teleaziz/next.js that referenced this pull request Jan 5, 2022
…o-example

* 'canary' of github.com:vercel/next.js:
  [chore] Update `deta` version in examples (vercel#30204)
  fix: typescript example supporting strict w/ version >= 4.4 (vercel#33042)
  Avoid page double render with emotion vanilla (vercel#30541)
  converted the old tailwind css example to typescript  (vercel#32808)
  fix(examples/cms-contentful): add correct Content-Type + missing closing tag for html (vercel#30321)
  Ensure NODE_ENV is not inlined for next/jest (vercel#33032)
  Rename api in with-redis example (vercel#33016)
  Remove unused turbo remote cache env vars (vercel#33030)
  v12.0.8-canary.17
  Re-enable turbo caching for swc build jobs (vercel#32617)
  feat(cli): introduce `next info` CLI command (vercel#32972)
  fix(examples): add missing dependencies wo (vercel#32977)
  Updated wrong link to example of gtag init in measuring-performance.md (vercel#32974)
  v12.0.8-canary.16
  Revert "Reduce install size for linux glibc/musl (vercel#32850)" (vercel#32973)
  Ensure middleware is output in standalone mode (vercel#32967)
  v12.0.8-canary.15
  Reduce install size for linux glibc/musl (vercel#32850)
  Fixes issue with makeStylesheetInert (vercel#32027)
  Ensure setImmediate and punycode are polyfilled (vercel#32768)
@vercel vercel locked as resolved and limited conversation to collaborators Feb 3, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants