Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
6 changes: 2 additions & 4 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 1 addition & 4 deletions packages/cache-utils/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -53,10 +53,7 @@
"cpy": "^11.0.0",
"get-stream": "^9.0.0",
"globby": "^14.0.0",
"junk": "^4.0.0",
"locate-path": "^7.0.0",
"move-file": "^3.0.0",
"readdirp": "^4.0.0"
"junk": "^4.0.0"
},
"devDependencies": {
"@types/node": "^22.12.0",
Expand Down
24 changes: 22 additions & 2 deletions packages/cache-utils/src/fs.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import { basename, dirname, join } from 'path'
import cpy from 'cpy'
import { type Options, globby } from 'globby'
import { isNotJunk } from 'junk'
import { moveFile } from 'move-file'

/**
* Move or copy a cached file/directory from/to a local one
Expand All @@ -15,7 +14,28 @@ import { moveFile } from 'move-file'
export const moveCacheFile = async function (src: string, dest: string, move = false) {
// Moving is faster but removes the source files locally
if (move) {
return moveFile(src, dest, { overwrite: false })
Comment thread
serhalp marked this conversation as resolved.
try {
await fs.access(dest)
throw new Error(`The destination file exists: ${dest}`)
} catch (error) {
if ((error as NodeJS.ErrnoException).code !== 'ENOENT') {
throw error
}
}

await fs.mkdir(dirname(dest), { recursive: true })

try {
await fs.rename(src, dest)
} catch (error) {
if ((error as NodeJS.ErrnoException).code !== 'EXDEV') {
throw error
}
// Only happens when moving cross-device
await fs.cp(src, dest, { recursive: true, force: false, errorOnExist: true })
await fs.rm(src, { recursive: true, force: true })
Comment thread
coderabbitai[bot] marked this conversation as resolved.
}
return
}

const { srcGlob, dest: matchedDest, ...options } = await getSrcAndDest(src, dirname(dest))
Expand Down
20 changes: 16 additions & 4 deletions packages/cache-utils/src/hash.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,7 @@
import { createHash } from 'crypto'
import { createReadStream } from 'fs'
import { createReadStream, promises as fs } from 'fs'

import getStream from 'get-stream'
import { locatePath } from 'locate-path'

// We need a hashing algorithm that's as fast as possible.
// Userland CRC32 implementations are actually slower than Node.js SHA1.
Expand All @@ -11,13 +10,26 @@ const HASH_ALGO = 'sha1'
// Caching a big directory like `node_modules` is slow. However, those can
// sometime be represented by a digest file such as `package-lock.json`. If this
// has not changed, we don't need to save cache again.
export const getHash = async function (digests, move) {
export const getHash = async function (digests: string[], move: boolean) {
// Moving files is faster than computing hashes
if (move || digests.length === 0) {
return
}

const digestPath = await locatePath(digests)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It looks like there are two behavioural changes here (see locate-path vs. fsPromises.access:

  1. By default, locate-path only matches on files, not directories.
  2. By default, paths are resolved from process.cwd.

Maybe we can just steal the logic from its source here.

Not sure if these are important (e.g. maybe directories aren't possible? maybe the paths are always absolute? no idea).

Maybe add some test coverage for this too?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

i feel like im missing something because this is same as my other comment:

By default, paths are resolved from process.cwd

just like all fs functions, no?

let digestPath: string | undefined = undefined

for (const digest of digests) {
try {
const stat = await fs.stat(digest)
if (stat.isFile()) {
digestPath = digest
break
}
} catch {
continue
}
}

if (digestPath === undefined) {
return
}
Expand Down
36 changes: 29 additions & 7 deletions packages/cache-utils/src/list.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { join } from 'path'

import { readdirpPromise } from 'readdirp'
import { join, relative } from 'path'
import { readdir } from 'node:fs/promises'
import type { Dirent } from 'node:fs'

import { getCacheDir } from './dir.js'
import { isManifest } from './manifest.js'
Expand Down Expand Up @@ -36,11 +36,33 @@ const listBase = async function ({
cacheDir: string
depth?: number
}) {
const files = await readdirpPromise(`${cacheDir}/${name}`, { fileFilter, depth, type: 'files_directories' })
const filesA = files.map(({ path }) => join(base, path))
return filesA
let queue = [{ path: `${cacheDir}/${name}`, depth: 0 }]
const results: string[] = []
let queueEntry: { path: string; depth: number } | undefined
while ((queueEntry = queue.pop())) {
const { path: currentPath, depth: currentDepth } = queueEntry
let children: Dirent[]
try {
children = await readdir(currentPath, { withFileTypes: true })
} catch (error) {
if ((error as NodeJS.ErrnoException).code === 'ENOENT') {
continue
}
throw error
}
for (const child of children) {
const childPath = join(child.parentPath, child.name)
if ((child.isDirectory() || child.isFile()) && fileFilter(child.name)) {
results.push(join(base, relative(`${cacheDir}/${name}`, childPath)))
}
if ((depth === undefined || currentDepth < depth) && child.isDirectory()) {
queue.push({ path: childPath, depth: currentDepth + 1 })
}
}
}
return results
Comment on lines +39 to +63

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If I'm reading all this right, I think this complexity isn't worthwhile, since the only reason we aren't using a simple readdirp('...', { recursive: true }) is because it doesn't support filtering on the fly, but since the fileFilter is actually just excluding basically one or zero files (no directories), it should be fine to filter after the fact, performance wise. What do you think?

Suggested change
let queue = [{ path: `${cacheDir}/${name}`, depth: 0 }]
const results: string[] = []
let queueEntry: { path: string; depth: number } | undefined
while ((queueEntry = queue.pop())) {
const { path: currentPath, depth: currentDepth } = queueEntry
let children: Dirent[]
try {
children = await readdir(currentPath, { withFileTypes: true })
} catch (error) {
if ((error as NodeJS.ErrnoException).code === 'ENOENT') {
continue
}
throw error
}
for (const child of children) {
const childPath = join(child.parentPath, child.name)
if ((child.isDirectory() || child.isFile()) && fileFilter(child.name)) {
results.push(join(base, relative(`${cacheDir}/${name}`, childPath)))
}
if ((depth === undefined || currentDepth < depth) && child.isDirectory()) {
queue.push({ path: childPath, depth: currentDepth + 1 })
}
}
}
return results
const results = await readdir(`${cacheDir}/${name}`, { recursive: true })
return results.filter(({ name }) => fileFilter(name))

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

i agree. will update 👍

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

👁️ bump!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

i dug into this a bit and its actually because we need depth control.

if we don't care about that anymore, we can collapse it down to what you suggested. i need to remind myself why we have a depth...

}

const fileFilter = function ({ basename }) {
const fileFilter = function (basename) {
return !isManifest(basename)
}
2 changes: 1 addition & 1 deletion packages/cache-utils/src/main.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ const saveOne = async function (
cwd?: string
move?: boolean
ttl?: number
digests?: any[]
digests?: string[]
} = {},
) {
const { srcPath, cachePath } = await parsePath({ path, cacheDir, cwdOpt })
Expand Down
17 changes: 17 additions & 0 deletions packages/cache-utils/tests/digests.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,23 @@ test('Should allow caching according to several potential digests files', async
}
})

test('Should ignore directory digests', async () => {
const [cacheDir, srcDir] = await Promise.all([createTmpDir(), createTmpDir()])
try {
const srcFile = `${srcDir}/test`
const digestDir = `${srcDir}/digestDir`
await Promise.all([fs.writeFile(srcFile, 'test'), fs.mkdir(digestDir)])
expect(await save(srcDir, { cacheDir, digests: [digestDir] })).toBe(true)
await fs.writeFile(srcFile, 'newTest')
expect(await save(srcDir, { cacheDir, digests: [digestDir] })).toBe(true)
expect(await restore(srcDir, { cacheDir, digests: [digestDir] })).toBe(true)
const content = await fs.readFile(srcFile, 'utf8')
expect(content).toBe('newTest')
} finally {
await removeFiles([cacheDir, srcDir])
}
})

test('Should ignore non-existing digests files', async () => {
const [cacheDir, srcDir] = await Promise.all([createTmpDir(), createTmpDir()])
try {
Expand Down
Loading