-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Fix: More resillient global and cache folder determination #4325
Changes from 14 commits
fa98ad9
f4b48aa
b1c43cc
a9b3542
e478af9
dcb953c
5685843
9576dbd
b9c0fb8
73c61ae
1e6b708
54c7581
86de168
89d52ce
f21abfb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,18 +1,18 @@ | ||
/* @flow */ | ||
|
||
import type {ReadStream} from 'fs'; | ||
|
||
import type Reporter from '../reporters/base-reporter.js'; | ||
|
||
import fs from 'fs'; | ||
import globModule from 'glob'; | ||
import os from 'os'; | ||
import path from 'path'; | ||
|
||
import BlockingQueue from './blocking-queue.js'; | ||
import * as promise from './promise.js'; | ||
import {promisify} from './promise.js'; | ||
import map from './map.js'; | ||
|
||
const fs = require('fs'); | ||
const globModule = require('glob'); | ||
const os = require('os'); | ||
const path = require('path'); | ||
|
||
export const constants = | ||
typeof fs.constants !== 'undefined' | ||
? fs.constants | ||
|
@@ -33,6 +33,7 @@ export const rename: (oldPath: string, newPath: string) => Promise<void> = promi | |
export const access: (path: string, mode?: number) => Promise<void> = promisify(fs.access); | ||
export const stat: (path: string) => Promise<fs.Stats> = promisify(fs.stat); | ||
export const unlink: (path: string) => Promise<void> = promisify(require('rimraf')); | ||
export const unlinkFile: (path: string) => Promise<void> = promisify(fs.unlink); | ||
export const mkdirp: (path: string) => Promise<void> = promisify(require('mkdirp')); | ||
export const exists: (path: string) => Promise<boolean> = promisify(fs.exists, true); | ||
export const lstat: (path: string) => Promise<fs.Stats> = promisify(fs.lstat); | ||
|
@@ -92,6 +93,16 @@ type CopyOptions = { | |
artifactFiles: Array<string>, | ||
}; | ||
|
||
type FailedFolderQuery = { | ||
error: Error, | ||
folder: string, | ||
}; | ||
|
||
type FolderQueryResult = { | ||
skipped: Array<FailedFolderQuery>, | ||
folder: ?string, | ||
}; | ||
|
||
export const fileDatesEqual = (a: Date, b: Date) => { | ||
const aTime = a.getTime(); | ||
const bTime = b.getTime(); | ||
|
@@ -880,3 +891,30 @@ export async function readFirstAvailableStream( | |
|
||
return {stream, triedPaths}; | ||
} | ||
|
||
export async function getFirstSuitableFolder( | ||
paths: Iterable<string>, | ||
mode: number = constants.W_OK | constants.X_OK, // eslint-disable-line no-bitwise | ||
): Promise<FolderQueryResult> { | ||
const result: FolderQueryResult = { | ||
skipped: [], | ||
folder: null, | ||
}; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why don't we just return a raw string? The list of skipped directories can be deduced relatively easily by the caller anyway. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because I also attach the error, in case that matters. May be it is EACCESS, may be it is EROFS or may be it is something else? Also deducing the list would require another traversal of the entire list until the selected folder is found which is a bit unnecessary. |
||
|
||
for (const folder of paths) { | ||
try { | ||
await mkdirp(folder); | ||
await access(folder, mode); | ||
|
||
result.folder = folder; | ||
|
||
return result; | ||
} catch (error) { | ||
result.skipped.push({ | ||
error, | ||
folder, | ||
}); | ||
} | ||
} | ||
return result; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's the problem with this code:
We only need write permissions when doing
yarn global add
which is usually called withsudo
or similar. That said if we change our permission checks depending on usage, then we may end up using two different global folders based on howyarn
is called (with sudo and without) which would make runningyarn global add
useless.I think we should just check
R_OK
andX_OK
here but doing that caused some failing tests (which can be fixed, I'm sure). This would also be a breaking change. That said I'm almost certain thatPOSIX_GLOBAL_PREFIX/bin
won't be writeable to yarn by default. What do you think?