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

[BUG] rm.all doesn't delete anything on Windows #165

Closed
1 task done
peterhirn opened this issue Feb 11, 2023 · 3 comments
Closed
1 task done

[BUG] rm.all doesn't delete anything on Windows #165

peterhirn opened this issue Feb 11, 2023 · 3 comments
Labels
Bug Needs Triage needs an initial review

Comments

@peterhirn
Copy link

peterhirn commented Feb 11, 2023

Is there an existing issue for this?

  • I have searched the existing issues

Current Behavior

  • version 17.0.4
  • node 19.6
  • this is a windows only bug, works as expected in node:19.6.0-alpine
import cacache from "cacache"

const cachePath = "./cache"
const key = "test-key"

await cacache.put(cachePath, key, "10293801983029384")

const cached = await cacache.get(cachePath, key)
console.log(`Data: ${cached.data}`)

await cacache.rm.all(cachePath)

const cached2 = await cacache.get(cachePath, key)
console.log(`Data after rm.all: ${cached2.data}`)

Output

Data: 10293801983029384
Data after rm.all: 10293801983029384

After rm.all cache keys still exist and nothing is delete from the filesystem.

Expected Behavior

Exception: NotFoundError: No cache entry for test-key found in ./cache

Works as expected in version 16.1.3

Steps To Reproduce

No response

Environment

  • Node: 19.6
  • OS: Windows 10
@peterhirn peterhirn added Bug Needs Triage needs an initial review labels Feb 11, 2023
@peterhirn peterhirn changed the title [BUG] rm.all doesn't delete anything [BUG] rm.all doesn't delete anything on Windows Feb 11, 2023
@peterhirn
Copy link
Author

peterhirn commented Feb 11, 2023

The problem is lib/util/glob.js

https://www.npmjs.com/package/glob#windows

The pattern argument is cache\*(content-*|index-*) on windows, cache/*(content-*|index-*) on linux.

Maybe globify should be something like this

const globify = (pattern) => pattern.replace(/\\+|\/{2,}/g, '/')

But I really don't understand why double forward slashes are replaced at all.

@supita
Copy link
Contributor

supita commented May 16, 2023

I'm having an issue where the stats from cacache.verify are not properly reported on Windows because the files are not detected.

The root of the problem is the same: globify function is not replacing to forward-slashes as glob library is expecting to handle Windows paths.

I found that before globify was at lib/verify.js, the replacement of the forward-slashes was made correctly.

const globify = pattern => pattern.split('\\').join('/')

But when created the file lib/util/glob.js and moved globify there, it was changed to

const globify = pattern => pattern.split('//').join('/')

which is causing problems when glob is used for obtaining stats and cleanup in the case of verify

wraithgar pushed a commit that referenced this issue May 18, 2023
## What

While testing `cacache.verify` I found that on Windows the `stats`
returned by it are not properly reported. Moreover, I found that
`cacache.verify` fails to delete files if no cache entries refer to
them.

`cacache.rm.all` also fails to work on Windows: the cache is not cleared
after calling this function.

## Why

The root of the problem is the same: `globify` function is not replacing
to forward-slashes as [`glob` library is expecting to handle Windows
paths](https://github.com/isaacs/node-glob#windows).

I found that originally `globify` was at `lib/verify.js`, the
replacement [of the forward-slashes was made
correctly](https://github.com/npm/cacache/pull/101/files#diff-eeac8636d9b6fc8549cda5929066bc44196ae8b9daac1f414fdd61e2e1340db6R16).

```javascript
const globify = pattern => pattern.split('\\').join('/')
```

But when created the file `lib/util/glob.js` and moved `globify` there,
it [was
changed](https://github.com/npm/cacache/pull/141/files#diff-20cceb346ffab6392d2b141017e1aa137df33c1c3ce59d5ccc765171586919f4R6)
to

```javascript
const globify = pattern => pattern.split('//').join('/')
```

which is causing problems when `glob` is used for obtaining stats and
cleanup in the case of
[`verify`](https://github.com/npm/cacache/blob/13a4ba3423d8d33b7d718e7200e5d3a5ec720a6d/lib/verify.js#L113)
and delete the cache with
[rm.all](https://github.com/npm/cacache/blob/13a4ba3423d8d33b7d718e7200e5d3a5ec720a6d/lib/rm.js#L29)

## Change details

Replace backslash by forward-slashes if they are present on a path
before calling `glob`.

Initially I was going to rollback the change to the first implementation
of `globify`, but I though it would be more explicit to use
[path.sep](https://nodejs.org/api/path.html#pathsep).

## Issues reported

[BUG] rm.all doesn't delete anything on Windows #165
@wraithgar
Copy link
Member

Thank you @supita
#209

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Needs Triage needs an initial review
Projects
None yet
Development

No branches or pull requests

3 participants