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

hash the font file name if it exceeds our max file size #69144

Open
wants to merge 1 commit into
base: arlyon/font-code-refactor
Choose a base branch
from
Open

hash the font file name if it exceeds our max file size #69144

wants to merge 1 commit into from

Conversation

arlyon
Copy link
Contributor

@arlyon arlyon commented Aug 21, 2024 •
edited

What?

When using google fonts, we import files and write them with their hash to the filesystem. Some of these fonts get very close to the file size limit which, after appending a hash onto them, causes turbo to fail with a write error.

Why?

Different operating systems have different path length requirements. In short:

  • macOS: limit per segment, longer limit for entire path length
  • unix: limit per segment, longer limit for entire path length
  • windows: hard limit of 260 chars unless using UNC

We need to respect these.

How?

A small change in the font code that rewrites filesystem paths if they approach this limit, with some buffer room. Rather than truncating I opted for a 64 bit hash because many fonts have long common prefixes. We could bump this up to 128 or higher but in practice there will be very very few files that need this code path.

Another option is a general handler for this where reads and writes are intercepted in turbo tasks fs but I wanted to achieve a few things:

  • explicitly handle sources of problematic file names at the source, rather than having a blanket impl
  • avoid obscure hashed names appearing in our outputs generally since it makes debugging harder
  • avoid lots of hashing on reads and writes

To me it is safer to have the source ensure they are meeting the needs of the system since we can control source specific implementations.

Closes PACK-3012

@ijjk ijjk added created-by: Turbopack team PRs by the Turbopack team. Font (next/font) Related to Next.js Font Optimization. Turbopack Related to Turbopack with Next.js. labels Aug 21, 2024
@arlyon Graphite App
Copy link
Contributor Author

arlyon commented Aug 21, 2024 •
edited

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @arlyon and the rest of your teammates on Graphite Graphite

@arlyon arlyon mentioned this pull request Aug 21, 2024
Open
@arlyon arlyon changed the title simplify font finding logic hash the font file name if it exceeds our max file size Aug 21, 2024
@arlyon arlyon marked this pull request as ready for review August 21, 2024 14:16
@arlyon arlyon force-pushed the arlyon/pack-3012-file-name-too-long-os-error-63-since-142 branch from 5f85b3b to 7951fad Compare August 21, 2024 14:20
@arlyon arlyon changed the base branch from arlyon/attempt-fix-too-long-file to arlyon/font-code-refactor August 21, 2024 14:20
@arlyon arlyon mentioned this pull request Aug 21, 2024
Open
@ijjk
Copy link
Member

ijjk commented Aug 21, 2024

Failing test suites

Commit: 5f85b3b

TURBOPACK=1 pnpm test-dev test/e2e/app-dir/app-compilation/index.test.ts (turbopack)

  • app dir > HMR > should not cause error when removing loading.js
Expand output

● app dir › HMR › should not cause error when removing loading.js

expect(received).toInclude(expected)

Expected string to include:
  "✓ Compiled"
Received:
  " ⨯ ./node_modules/.pnpm/file+..+next-repo-de3d7d811818aef9986d348de979bf8c122a5cff323da81f0631c620bdbbea01+packages+n_klwh2mzcek5vdfkuurc335kn4y/node_modules/next/dist/esm/build/templates/app-page.js:7:4
Module not found: Can't resolve 'COMPONENT_3'··
https://nextjs.org/docs/messages/module-not-found··
 ⨯ ./node_modules/.pnpm/file+..+next-repo-de3d7d811818aef9986d348de979bf8c122a5cff323da81f0631c620bdbbea01+packages+n_klwh2mzcek5vdfkuurc335kn4y/node_modules/next/dist/esm/build/templates/app-page.js:7:4
Module not found: Can't resolve 'COMPONENT_3'··
https://nextjs.org/docs/messages/module-not-found··
 ⨯ ./node_modules/.pnpm/file+..+next-repo-de3d7d811818aef9986d348de979bf8c122a5cff323da81f0631c620bdbbea01+packages+n_klwh2mzcek5vdfkuurc335kn4y/node_modules/next/dist/esm/build/templates/app-page.js:7:4
Module not found: Can't resolve 'COMPONENT_3'··
https://nextjs.org/docs/messages/module-not-found··
 ⨯ ./node_modules/.pnpm/file+..+next-repo-de3d7d811818aef9986d348de979bf8c122a5cff323da81f0631c620bdbbea01+packages+n_klwh2mzcek5vdfkuurc335kn4y/node_modules/next/dist/esm/build/templates/app-page.js:7:4
Module not found: Can't resolve 'COMPONENT_3'··
https://nextjs.org/docs/messages/module-not-found··
"

  38 |
  39 |         await retry(async () => {
> 40 |           expect(next.cliOutput.slice(cliOutputLength)).toInclude('✓ Compiled')
     |                                                         ^
  41 |         })
  42 |
  43 |         // It should not have an error

  at toInclude (e2e/app-dir/app-compilation/index.test.ts:40:57)
  at fn (lib/next-test-utils.ts:806:20)
  at Object.<anonymous> (e2e/app-dir/app-compilation/index.test.ts:39:9)

Read more about building and testing Next.js in contributing.md.

@arlyon arlyon requested review from wbinnssmith and bgw August 23, 2024 17:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wbinnssmith wbinnssmith wbinnssmith approved these changes

@bgw bgw Awaiting requested review from bgw

Assignees
No one assigned
Labels
created-by: Turbopack team PRs by the Turbopack team. Font (next/font) Related to Next.js Font Optimization. Turbopack Related to Turbopack with Next.js.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@arlyon @ijjk @wbinnssmith