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

Fix: More resillient global and cache folder determination #4325

Merged
merged 15 commits into from
Sep 7, 2017
31 changes: 16 additions & 15 deletions src/cli/commands/global.js
Original file line number Diff line number Diff line change
Expand Up @@ -87,29 +87,30 @@ async function getGlobalPrefix(config: Config, flags: Object): Promise<string> {
return process.env.PREFIX;
}

let prefix = FALLBACK_GLOBAL_PREFIX;
const potentialPrefixFolders = [
FALLBACK_GLOBAL_PREFIX,
// This is the last resort
path.dirname(process.execPath),
];
if (process.platform === 'win32') {
// %LOCALAPPDATA%\Yarn --> C:\Users\Alice\AppData\Local\Yarn
if (process.env.LOCALAPPDATA) {
prefix = path.join(process.env.LOCALAPPDATA, 'Yarn');
potentialPrefixFolders.unshift(path.join(process.env.LOCALAPPDATA, 'Yarn'));
}
} else {
prefix = POSIX_GLOBAL_PREFIX;
potentialPrefixFolders.unshift(POSIX_GLOBAL_PREFIX);
}

const binFolder = path.join(prefix, 'bin');
try {
// eslint-disable-next-line no-bitwise
await fs.access(binFolder, fs.constants.W_OK | fs.constants.X_OK);
} catch (err) {
if (err.code === 'EACCES') {
prefix = FALLBACK_GLOBAL_PREFIX;
} else if (err.code === 'ENOENT') {
// ignore - that just means we don't have the folder, yet
} else {
throw err;
}
const binFolders = potentialPrefixFolders.map(prefix => path.join(prefix, 'bin'));
const prefixFolderQueryResult = await fs.getFirstWriteableFolder(binFolders);
const prefix = prefixFolderQueryResult.folder && path.dirname(prefixFolderQueryResult.folder);

if (!prefix) {
throw new MessageError(
config.reporter.lang('noGlobalFolder', prefixFolderQueryResult.skipped.map(item => item.folder).join(', ')),
);
}

return prefix;
}

Expand Down
28 changes: 8 additions & 20 deletions src/config.js
Original file line number Diff line number Diff line change
Expand Up @@ -290,29 +290,17 @@ export default class Config {
const preferredCacheFolder = opts.preferredCacheFolder || this.getOption('preferred-cache-folder', true);

if (preferredCacheFolder) {
preferredCacheFolders = [preferredCacheFolder].concat(preferredCacheFolders);
preferredCacheFolders = [String(preferredCacheFolder)].concat(preferredCacheFolders);
}

for (let t = 0; t < preferredCacheFolders.length && !cacheRootFolder; ++t) {
const tentativeCacheFolder = String(preferredCacheFolders[t]);

try {
await fs.mkdirp(tentativeCacheFolder);

const testFile = path.join(tentativeCacheFolder, 'testfile');

// fs.access is not enough, because the cache folder could actually be a file.
await fs.writeFile(testFile, 'content');
await fs.readFile(testFile);

cacheRootFolder = tentativeCacheFolder;
} catch (error) {
this.reporter.warn(this.reporter.lang('cacheFolderSkipped', tentativeCacheFolder));
}
const cacheFolderQuery = await fs.getFirstWriteableFolder(preferredCacheFolders);
for (const failedFolder of cacheFolderQuery.skipped) {
this.reporter.warn(this.reporter.lang('cacheFolderSkipped', failedFolder));
Copy link
Member

Choose a reason for hiding this comment

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

failedFolder is not a string, so the display will probably be wrong. I think you meant failedFolder.folder? The variable name would probably be better as skippedEntry btw, so that it would be skippedEntry.folder

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup to both, good catch!

}

if (cacheRootFolder && t > 0) {
this.reporter.warn(this.reporter.lang('cacheFolderSelected', cacheRootFolder));
}
cacheRootFolder = cacheFolderQuery.folder;
if (cacheRootFolder && cacheFolderQuery.skipped.length > 0) {
this.reporter.warn(this.reporter.lang('cacheFolderSelected', cacheRootFolder));
}
}

Expand Down
2 changes: 1 addition & 1 deletion src/constants.js
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ function getYarnBinPath(): string {
export const NODE_MODULES_FOLDER = 'node_modules';
export const NODE_PACKAGE_JSON = 'package.json';

export const POSIX_GLOBAL_PREFIX = '/usr/local';
export const POSIX_GLOBAL_PREFIX = `${process.env.DESTIR || ''}/usr/local`;
Copy link
Member

Choose a reason for hiding this comment

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

typo

Copy link
Member

Choose a reason for hiding this comment

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

(DESTIR instead of DESTDIR)

Copy link
Member Author

Choose a reason for hiding this comment

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

darn

export const FALLBACK_GLOBAL_PREFIX = path.join(userHome, '.yarn');

export const META_FOLDER = '.yarn-meta';
Expand Down
1 change: 1 addition & 0 deletions src/reporters/lang/en.js
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,7 @@ const messages = {
unexpectedError: 'An unexpected error occurred: $0.',
jsonError: 'Error parsing JSON at $0, $1.',
noPermission: 'Cannot create $0 due to insufficient permissions.',
noGlobalFolder: 'Cannot find a suitable global folder. Tried these: $0',
allDependenciesUpToDate: 'All of your dependencies are up to date.',
legendColorsForUpgradeInteractive:
'Color legend : \n $0 : Major Update backward-incompatible updates \n $1 : Minor Update backward-compatible features \n $2 : Patch Update backward-compatible bug fixes',
Expand Down
49 changes: 43 additions & 6 deletions src/util/fs.js
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
Expand Down Expand Up @@ -92,6 +92,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();
Expand Down Expand Up @@ -880,3 +890,30 @@ export async function readFirstAvailableStream(

return {stream, triedPaths};
}

export async function getFirstWriteableFolder(paths: Iterable<string>): Promise<FolderQueryResult> {
const result: FolderQueryResult = {
skipped: [],
folder: null,
};
Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Member Author

@BYK BYK Sep 6, 2017

Choose a reason for hiding this comment

The 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);
const testFile = path.join(folder, `.yarn-write-test-${process.pid}`);
await writeFile(testFile, '');
await readFile(testFile);
await unlink(testFile);

result.folder = folder;

return result;
} catch (error) {
result.skipped.push({
error,
folder,
});
}
}
return result;
}