Skip to content

Commit e64d69a

Browse files
lukekarryslukekarrys
committed
feat: write eresolve error files to the logs directory
Also refactor all files written to the logs directory to use the same code path for file name creation.
1 parent 3445da0 commit e64d69a

16 files changed

Lines changed: 192 additions & 234 deletions

‎lib/npm.js‎

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -18,11 +18,9 @@ const replaceInfo = require('./utils/replace-info.js')
1818
const updateNotifier = require('./utils/update-notifier.js')
1919
const pkg = require('../package.json')
2020
const cmdList = require('./utils/cmd-list.js')
21-
const runId = require('./utils/run-id.js')
2221

2322
let warnedNonDashArg = false
2423
const _load = Symbol('_load')
25-
const RUN_ID = runId()
2624

2725
class Npm extends EventEmitter {
2826
static get version () {
@@ -34,16 +32,16 @@ class Npm extends EventEmitter {
3432
loadErr = null
3533
argv = []
3634

35+
#runId = new Date().toISOString().replace(/[.:]/g, '_')
3736
#loadPromise = null
3837
#tmpFolder = null
3938
#title = 'npm'
4039
#argvClean = []
4140
#chalk = null
4241

43-
#logFile = new LogFile({ id: RUN_ID })
42+
#logFile = new LogFile()
4443
#display = new Display()
4544
#timers = new Timers({
46-
id: RUN_ID,
4745
start: 'npm',
4846
listener: (name, ms) => {
4947
const args = ['timing', name, `Completed in ${ms}ms`]
@@ -209,6 +207,7 @@ class Npm extends EventEmitter {
209207

210208
writeTimingFile () {
211209
this.#timers.writeFile({
210+
id: this.#runId,
212211
command: this.#argvClean,
213212
logfiles: this.logFiles,
214213
version: this.version,
@@ -289,15 +288,15 @@ class Npm extends EventEmitter {
289288

290289
this.time('npm:load:logFile', () => {
291290
this.#logFile.load({
292-
dir: this.logsDir,
291+
path: this.logPath,
293292
logsMax: this.config.get('logs-max'),
294293
})
295294
log.verbose('logfile', this.#logFile.files[0] || 'no logfile created')
296295
})
297296

298297
this.time('npm:load:timers', () =>
299298
this.#timers.load({
300-
dir: this.config.get('timing') ? this.logsDir : null,
299+
path: this.config.get('timing') ? this.logPath : null,
301300
})
302301
)
303302

@@ -371,6 +370,10 @@ class Npm extends EventEmitter {
371370
return this.config.get('logs-dir') || join(this.cache, '_logs')
372371
}
373372

373+
get logPath () {
374+
return resolve(this.logsDir, `${this.#runId}-`)
375+
}
376+
374377
get timingFile () {
375378
return this.#timers.file
376379
}

‎lib/utils/error-message.js‎

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ const log = require('./log-shim')
88
module.exports = (er, npm) => {
99
const short = []
1010
const detail = []
11+
const files = []
1112

1213
if (er.message) {
1314
er.message = replaceInfo(er.message)
@@ -17,14 +18,17 @@ module.exports = (er, npm) => {
1718
}
1819

1920
switch (er.code) {
20-
case 'ERESOLVE':
21+
case 'ERESOLVE': {
2122
short.push(['ERESOLVE', er.message])
2223
detail.push(['', ''])
2324
// XXX(display): error messages are logged so we use the logColor since that is based
2425
// on stderr. This should be handled solely by the display layer so it could also be
2526
// printed to stdout if necessary.
26-
detail.push(['', report(er, !!npm.logColor, resolve(npm.cache, 'eresolve-report.txt'))])
27+
const { explanation, file } = report(er, !!npm.logColor)
28+
detail.push(['', explanation])
29+
files.push(['eresolve-report.txt', file])
2730
break
31+
}
2832

2933
case 'ENOLOCK': {
3034
const cmd = npm.command || ''
@@ -398,5 +402,5 @@ module.exports = (er, npm) => {
398402

399403
break
400404
}
401-
return { summary: short, detail: detail }
405+
return { summary: short, detail, files }
402406
}

‎lib/utils/exit-handler.js‎

Lines changed: 25 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
const os = require('os')
2+
const fs = require('@npmcli/fs')
23

34
const log = require('./log-shim.js')
45
const errorMessage = require('./error-message.js')
@@ -18,11 +19,10 @@ process.on('exit', code => {
1819
// unfinished timer check below
1920
process.emit('timeEnd', 'npm')
2021

21-
const hasNpm = !!npm
22-
const hasLoadedNpm = hasNpm && npm.config.loaded
22+
const hasLoadedNpm = npm?.config.loaded
2323

2424
// Unfinished timers can be read before config load
25-
if (hasNpm) {
25+
if (npm) {
2626
for (const [name, timer] of npm.unfinishedTimers) {
2727
log.verbose('unfinished npm timer', name, timer)
2828
}
@@ -111,10 +111,9 @@ const exitHandler = err => {
111111

112112
log.disableProgress()
113113

114-
const hasNpm = !!npm
115-
const hasLoadedNpm = hasNpm && npm.config.loaded
114+
const hasLoadedNpm = npm?.config.loaded
116115

117-
if (!hasNpm) {
116+
if (!npm) {
118117
err = err || new Error('Exit prior to setting npm in exit handler')
119118
// eslint-disable-next-line no-console
120119
console.error(err.stack || err.message)
@@ -181,17 +180,33 @@ const exitHandler = err => {
181180
}
182181
}
183182

184-
const msg = errorMessage(err, npm)
185-
for (const errline of [...msg.summary, ...msg.detail]) {
183+
const { summary, detail, files = [] } = errorMessage(err, npm)
184+
185+
for (let [file, content] of files) {
186+
file = `${npm.logPath}${file}`
187+
content = `'Log files:\n${npm.logFiles.join('\n')}\n\n${content.trim()}\n`
188+
try {
189+
fs.withOwnerSync(
190+
file,
191+
() => fs.writeFileSync(file, content),
192+
{ owner: 'inherit' }
193+
)
194+
detail.push(['', `\n\nFor a full report see:\n${file}`])
195+
} catch (err) {
196+
log.warn('', `Could not write error message to ${file} due to ${err}`)
197+
}
198+
}
199+
200+
for (const errline of [...summary, ...detail]) {
186201
log.error(...errline)
187202
}
188203

189204
if (hasLoadedNpm && npm.config.get('json')) {
190205
const error = {
191206
error: {
192207
code: err.code,
193-
summary: messageText(msg.summary),
194-
detail: messageText(msg.detail),
208+
summary: messageText(summary),
209+
detail: messageText(detail),
195210
},
196211
}
197212
npm.outputError(JSON.stringify(error, null, 2))

‎lib/utils/explain-eresolve.js‎

Lines changed: 16 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,6 @@
11
// this is called when an ERESOLVE error is caught in the exit-handler,
22
// or when there's a log.warn('eresolve', msg, explanation), to turn it
33
// into a human-intelligible explanation of what's wrong and how to fix.
4-
const { writeFileSync } = require('fs')
54
const { explainEdge, explainNode, printNode } = require('./explain-dep.js')
65

76
// expl is an explanation object that comes from Arborist. It looks like:
@@ -45,27 +44,25 @@ const explain = (expl, color, depth) => {
4544
}
4645

4746
// generate a full verbose report and tell the user how to fix it
48-
const report = (expl, color, fullReport) => {
49-
const orNoStrict = expl.strictPeerDeps ? '--no-strict-peer-deps, ' : ''
50-
const fix = `Fix the upstream dependency conflict, or retry
51-
this command with ${orNoStrict}--force, or --legacy-peer-deps
52-
to accept an incorrect (and potentially broken) dependency resolution.`
53-
54-
writeFileSync(fullReport, `# npm resolution error report
55-
56-
${new Date().toISOString()}
57-
58-
${explain(expl, false, Infinity)}
47+
const report = (expl, color) => {
48+
const flags = [
49+
expl.strictPeerDeps ? '--no-strict-peer-deps' : '',
50+
'--force',
51+
'--legacy-peer-deps',
52+
].filter(Boolean)
5953

60-
${fix}
54+
const or = (arr) => arr.length <= 2
55+
? arr.join(' or ') :
56+
arr.map((v, i, l) => i + 1 === l.length ? `or ${v}` : v).join(', ')
6157

62-
Raw JSON explanation object:
63-
64-
${JSON.stringify(expl, null, 2)}
65-
`, 'utf8')
58+
const fix = `Fix the upstream dependency conflict, or retry
59+
this command with ${or(flags)}
60+
to accept an incorrect (and potentially broken) dependency resolution.`
6661

67-
return explain(expl, color, 4) +
68-
`\n\n${fix}\n\nSee ${fullReport} for a full report.`
62+
return {
63+
explanation: `${explain(expl, color, 4)}\n\n${fix}`,
64+
file: `# npm resolution error report\n\n${explain(expl, false, Infinity)}\n\n${fix}`,
65+
}
6966
}
7067

7168
module.exports = {

‎lib/utils/log-file.js‎

Lines changed: 5 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -7,16 +7,11 @@ const MiniPass = require('minipass')
77
const fsMiniPass = require('fs-minipass')
88
const fs = require('@npmcli/fs')
99
const log = require('./log-shim')
10-
const runId = require('./run-id')
1110

1211
const padZero = (n, length) => n.toString().padStart(length.toString().length, '0')
1312
const globify = pattern => pattern.split('\\').join('/')
1413

1514
class LogFiles {
16-
// If we write multiple log files we want them all to have the same
17-
// identifier for sorting and matching purposes
18-
#logId = null
19-
2015
// Default to a plain minipass stream so we can buffer
2116
// initial writes before we know the cache location
2217
#logStream = null
@@ -34,16 +29,14 @@ class LogFiles {
3429

3530
#fileLogCount = 0
3631
#totalLogCount = 0
37-
#dir = null
32+
#path = null
3833
#logsMax = null
3934
#files = []
4035

4136
constructor ({
42-
id = runId(),
4337
maxLogsPerFile = 50_000,
4438
maxFilesPerProcess = 5,
4539
} = {}) {
46-
this.#logId = id
4740
this.#MAX_LOGS_PER_FILE = maxLogsPerFile
4841
this.#MAX_FILES_PER_PROCESS = maxFilesPerProcess
4942
this.on()
@@ -73,18 +66,18 @@ class LogFiles {
7366
this.#endStream()
7467
}
7568

76-
load ({ dir, logsMax = Infinity } = {}) {
69+
load ({ path, logsMax = Infinity } = {}) {
7770
// dir is user configurable and is required to exist so
7871
// this can error if the dir is missing or not configured correctly
79-
this.#dir = dir
72+
this.#path = path
8073
this.#logsMax = logsMax
8174

8275
// Log stream has already ended
8376
if (!this.#logStream) {
8477
return
8578
}
8679

87-
log.verbose('logfile', `logs-max:${logsMax} dir:${dir}`)
80+
log.verbose('logfile', `logs-max:${logsMax} dir:${this.#path}`)
8881

8982
// Pipe our initial stream to our new file stream and
9083
// set that as the new log logstream for future writes
@@ -164,7 +157,7 @@ class LogFiles {
164157
}
165158

166159
#getLogFilePath (count = '') {
167-
return path.resolve(this.#dir, `${this.#logId}-debug-${count}.log`)
160+
return `${this.#path}debug-${count}.log`
168161
}
169162

170163
#openLogFile () {

‎lib/utils/run-id.js‎

Lines changed: 0 additions & 3 deletions
This file was deleted.

‎lib/utils/timers.js‎

Lines changed: 5 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1,25 +1,21 @@
11
const EE = require('events')
2-
const { resolve } = require('path')
32
const fs = require('@npmcli/fs')
43
const log = require('./log-shim')
5-
const runId = require('./run-id')
64

75
// This is an event emiiter but on/off
86
// only listen on a single internal event that gets
97
// emitted whenever a timer ends
108
class Timers extends EE {
119
file = null
1210

13-
#id = null
1411
#unfinished = new Map()
1512
#finished = {}
1613
#onTimeEnd = Symbol('onTimeEnd')
1714
#initialListener = null
1815
#initialTimer = null
1916

20-
constructor ({ id = runId(), listener = null, start = 'npm' } = {}) {
17+
constructor ({ listener = null, start = 'npm' } = {}) {
2118
super()
22-
this.#id = id
2319
this.#initialListener = listener
2420
this.#initialTimer = start
2521
this.#init()
@@ -71,9 +67,9 @@ class Timers extends EE {
7167
return end
7268
}
7369

74-
load ({ dir } = {}) {
75-
if (dir) {
76-
this.file = resolve(dir, `${this.#id}-timing.json`)
70+
load ({ path } = {}) {
71+
if (path) {
72+
this.file = `${path}timing.json`
7773
}
7874
}
7975

@@ -86,10 +82,7 @@ class Timers extends EE {
8682
const globalStart = this.started
8783
const globalEnd = this.#finished.npm || Date.now()
8884
const content = {
89-
metadata: {
90-
id: this.#id,
91-
...metadata,
92-
},
85+
metadata,
9386
timers: this.#finished,
9487
// add any unfinished timers with their relative start/end
9588
unfinishedTimers: [...this.#unfinished.entries()].reduce((acc, [name, start]) => {

0 commit comments

Comments
 (0)