Skip to content

Commit

Permalink
fix: don't fail immediately if cache dir is not accessible
Browse files Browse the repository at this point in the history
This also changes all the log messages about not being able to create
initial directories and files to `log.verbose` since we know run those
commands on init. There are a lot of valid reasons why those might fail,
and we don't want to show a warning for them every time.

Fixes: #4769
Fixes: #4838
Fixes: #4996
  • Loading branch information
lukekarrys committed Jul 20, 2022
1 parent 51b12a0 commit 0c1e58f
Show file tree
Hide file tree
Showing 3 changed files with 50 additions and 21 deletions.
10 changes: 6 additions & 4 deletions lib/npm.js
Original file line number Diff line number Diff line change
Expand Up @@ -241,16 +241,18 @@ class Npm extends EventEmitter {
await this.time('npm:load:configload', () => this.config.load())

// mkdir this separately since the logs dir can be set to
// a different location. an error here should be surfaced
// right away since it will error in cacache later
// a different location. if this fails, then we don't have
// a cache dir, but we don't want to fail immediately since
// the command might not need a cache dir (like `npm --version`)
await this.time('npm:load:mkdirpcache', () =>
fs.mkdir(this.cache, { recursive: true, owner: 'inherit' }))
fs.mkdir(this.cache, { recursive: true, owner: 'inherit' })
.catch((e) => log.verbose('cache', `could not create cache: ${e}`)))

// its ok if this fails. user might have specified an invalid dir
// which we will tell them about at the end
await this.time('npm:load:mkdirplogs', () =>
fs.mkdir(this.logsDir, { recursive: true, owner: 'inherit' })
.catch((e) => log.warn('logfile', `could not create logs-dir: ${e}`)))
.catch((e) => log.verbose('logfile', `could not create logs-dir: ${e}`)))

// note: this MUST be shorter than the actual argv length, because it
// uses the same memory, so node will truncate it if it's too long.
Expand Down
6 changes: 4 additions & 2 deletions lib/utils/log-file.js
Original file line number Diff line number Diff line change
Expand Up @@ -204,7 +204,9 @@ class LogFiles {
this.#files.push(logStream.path)
return logStream
} catch (e) {
log.warn('logfile', `could not be created: ${e}`)
// If the user has a readonly logdir then we don't want to
// warn this on every command so it should be verbose
log.verbose('logfile', `could not be created: ${e}`)
}
}

Expand All @@ -226,7 +228,7 @@ class LogFiles {
)

// Always ignore the currently written files
const files = await glob(globify(logGlob), { ignore: this.#files.map(globify) })
const files = await glob(globify(logGlob), { ignore: this.#files.map(globify), silent: true })
const toDelete = files.length - this.#logsMax

if (toDelete <= 0) {
Expand Down
55 changes: 40 additions & 15 deletions test/lib/npm.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,9 @@ const { resolve, dirname, join } = require('path')

const { load: loadMockNpm } = require('../fixtures/mock-npm.js')
const mockGlobals = require('../fixtures/mock-globals')
const fs = require('@npmcli/fs')

const getMode = (f) => (fs.statSync(f).mode & parseInt('777', '8')).toString(8)

// delete this so that we don't have configs from the fact that it
// is being run by 'npm test'
Expand Down Expand Up @@ -435,23 +438,45 @@ t.test('debug log', async t => {
t.match(debug, log2.join(' '), 'after load log appears')
})

t.test('with bad dir', async t => {
const { npm } = await loadMockNpm(t, {
config: {
'logs-dir': 'LOGS_DIR',
},
mocks: {
'@npmcli/fs': {
mkdir: async (dir) => {
if (dir.includes('LOGS_DIR')) {
throw new Error('err')
}
},
},
},
t.test('can load with bad dir', async t => {
const { npm, cache } = await loadMockNpm(t, {
load: false,
})

t.equal(npm.logFiles.length, 0, 'no log file')
const logsDir = join(cache, '_logs')

// make inaccessible logs dir before load
await fs.mkdir(logsDir, { recursive: true, mode: '400' })

await t.resolves(npm.load(), 'loads with 400 logs dir')

t.equal(npm.logFiles.length, 0, 'no log files array')
t.strictSame(fs.readdirSync(logsDir), [], 'no actual log files')
t.equal(getMode(logsDir), '400')
})
})

t.test('cache dir', async t => {
t.test('creates a cache dir', async t => {
const { npm } = await loadMockNpm(t)

t.ok(fs.existsSync(npm.cache), 'cache dir exists')
})

t.test('can load with bad dir', async t => {
const { npm, cache } = await loadMockNpm(t, {
load: false,
})

// rm cache dir since it is created by mockNpm
// then recreate inaccessible cache dir
await fs.rm(cache, { recursive: true, force: true })
await fs.mkdir(cache, { recursive: true, mode: '400' })

await t.resolves(npm.load(), 'loads with 400 cache dir')

t.strictSame(fs.readdirSync(cache), [], 'nothing in cache dir')
t.equal(getMode(cache), '400')
})
})

Expand Down

0 comments on commit 0c1e58f

Please sign in to comment.