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

Fallbacks to /tmp if the preferred cache folder isn't writable #4143

Merged
merged 9 commits into from
Aug 17, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 36 additions & 0 deletions __tests__/integration.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@
import execa from 'execa';
import makeTemp from './_temp.js';
import * as fs from '../src/util/fs.js';
import * as misc from '../src/util/misc.js';
import * as constants from '../src/constants.js';

jasmine.DEFAULT_TIMEOUT_INTERVAL = 90000;

Expand Down Expand Up @@ -70,3 +72,37 @@ test('--mutex network', async () => {
execa(command, ['add', 'foo'].concat(args), options),
]);
});

test('cache folder fallback', async () => {
Copy link
Member

Choose a reason for hiding this comment

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

There are no expect statements here and it is not clear what this test is testing. My guess is "don't fail when preferred cache folder cannot be used" but it'd be nice to make that more visible. We should also, probably make sure the warning is printed.

const cwd = await makeTemp();
const cacheFolder = path.join(cwd, '.cache');

const command = path.resolve(__dirname, '../bin/yarn');
const args = ['--preferred-cache-folder', cacheFolder];

const options = {cwd};

function runCacheDir(): Promise<Array<Buffer>> {
const {stderr, stdout} = execa(command, ['cache', 'dir'].concat(args), options);

const stdoutPromise = misc.consumeStream(stdout);
const stderrPromise = misc.consumeStream(stderr);

return Promise.all([stdoutPromise, stderrPromise]);
}

const [stdoutOutput, stderrOutput] = await runCacheDir();

expect(stdoutOutput.toString().trim()).toEqual(path.join(cacheFolder, `v${constants.CACHE_VERSION}`));
expect(stderrOutput.toString()).not.toMatch(/Skipping preferred cache folder/);

await fs.unlink(cacheFolder);
await fs.writeFile(cacheFolder, `not a directory`);

const [stdoutOutput2, stderrOutput2] = await runCacheDir();

expect(stdoutOutput2.toString().trim()).toEqual(
path.join(constants.PREFERRED_MODULE_CACHE_DIRECTORIES[0], `v${constants.CACHE_VERSION}`),
);
expect(stderrOutput2.toString()).toMatch(/Skipping preferred cache folder/);
});
2 changes: 1 addition & 1 deletion src/cli/commands/cache.js
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ export const {run, setFlags, examples} = buildSubCommands('cache', {
},

dir(config: Config, reporter: Reporter) {
reporter.log(config.cacheFolder);
reporter.log(config.cacheFolder, {force: true});
Copy link
Member

Choose a reason for hiding this comment

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

I am not happy with this solution since I think it needs a better overall solution as we have discussed. Let's have a TODO comment here saying "Don't use force option and fix this ASAP" or something along those lines so we don't forget?

},

async clean(config: Config, reporter: Reporter, flags: Object, args: Array<string>): Promise<void> {
Expand Down
4 changes: 3 additions & 1 deletion src/cli/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,8 @@ export function main({
'--modules-folder <path>',
'rather than installing modules into the node_modules folder relative to the cwd, output them here',
);
commander.option('--cache-folder <path>', 'specify a custom folder to store the yarn cache');
commander.option('--preferred-cache-folder <path>', 'specify a custom folder to store the yarn cache if possible');
Copy link
Member

Choose a reason for hiding this comment

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

Better to say "prefer a custom cache folder when it is available". We can make this change in a follow-up.

commander.option('--cache-folder <path>', 'specify a custom folder that must be used to store the yarn cache');
Copy link
Member

Choose a reason for hiding this comment

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

A+ with the added emphasis with "must be". 👍

commander.option('--mutex <type>[:specifier]', 'use a mutex to ensure only one yarn instance is executing');
commander.option('--emoji [bool]', 'enable emoji in output', process.platform === 'darwin');
commander.option('-s, --silent', 'skip Yarn console logs, other types of logs (script output) will be printed');
Expand Down Expand Up @@ -324,6 +325,7 @@ export function main({
binLinks: commander.binLinks,
modulesFolder: commander.modulesFolder,
globalFolder: commander.globalFolder,
preferredCacheFolder: commander.preferredCacheFolder,
cacheFolder: commander.cacheFolder,
preferOffline: commander.preferOffline,
captureHar: commander.har,
Expand Down
44 changes: 41 additions & 3 deletions src/config.js
Original file line number Diff line number Diff line change
Expand Up @@ -276,9 +276,47 @@ export default class Config {
networkConcurrency: this.networkConcurrency,
networkTimeout: this.networkTimeout,
});
this._cacheRootFolder = String(
opts.cacheFolder || this.getOption('cache-folder', true) || constants.MODULE_CACHE_DIRECTORY,
);

let cacheRootFolder = opts.cacheFolder || this.getOption('cache-folder', true);
Copy link
Member

Choose a reason for hiding this comment

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

This should be pushed to the top of the list we try and we should fall back to tmp at all times for resilliency. Right now this override seems to be ignored.

Copy link
Member Author

Choose a reason for hiding this comment

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

I disagree - if the explicitely specified cache folder cannot be used, that's an hard error. This option contract forces us to use this folder. By (almost) silently using a different directory, we might end up with subtle bugs. For example, two Yarn process might run concurrently with different caches. In such a scenario, if for some reason their respective cache could not be reached, they would end up corrupting themselves.

Copy link
Member

Choose a reason for hiding this comment

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

For the readers of this thread, if there are any: we have talked about getting a unique folder once we fallback to tmp like by using a timestamp or the PID of the process, or both, but then decided to do this: #4143 (comment)

We should still probably try to ensure a unique folder in tmp when we fallback for safety.


if (!cacheRootFolder) {
let preferredCacheFolders = constants.PREFERRED_MODULE_CACHE_DIRECTORIES;
const preferredCacheFolder = opts.preferredCacheFolder || this.getOption('preferred-cache-folder', true);

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

for (let t = 0; t < preferredCacheFolders.length && !cacheRootFolder; ++t) {
Copy link
Member

Choose a reason for hiding this comment

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

Let's just use i when it is available in loops? :D

const tentativeCacheFolder = String(preferredCacheFolders[t]);
Copy link
Member

Choose a reason for hiding this comment

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

I'm guessing this is to silence Flow. Note to self: look into this since it may be pointing to a possibly undefined value which would break Yarn.


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);
await fs.unlink(testFile);

cacheRootFolder = tentativeCacheFolder;
} catch (error) {
this.reporter.warn(this.reporter.lang('cacheFolderSkipped', tentativeCacheFolder));
}

if (cacheRootFolder && t > 0) {
this.reporter.warn(this.reporter.lang('cacheFolderSelected', cacheRootFolder));
Copy link
Member

Choose a reason for hiding this comment

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

Note to self: make this information an info or debug level message for all cases to signal which cache folder is being used for each run.

}
}
}

if (!cacheRootFolder) {
throw new MessageError(this.reporter.lang('cacheFolderMissing'));
} else {
this._cacheRootFolder = String(cacheRootFolder);
Copy link
Member

Choose a reason for hiding this comment

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

Why do you need the String() casting?

Copy link
Member Author

Choose a reason for hiding this comment

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

Was there before. Probably because of Flow, I'd guess :)

Copy link
Member

Choose a reason for hiding this comment

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

May be we can remove it now?

Copy link
Member Author

Choose a reason for hiding this comment

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

Just tried, still fails.

}

this.workspacesEnabled = Boolean(this.getOption('workspaces-experimental'));

this.pruneOfflineMirror = Boolean(this.getOption('yarn-offline-mirror-pruning'));
Expand Down
16 changes: 12 additions & 4 deletions src/constants.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
/* @flow */

const os = require('os');
const path = require('path');
const userHome = require('./util/user-home-dir').default;

Expand Down Expand Up @@ -46,15 +47,21 @@ function getDirectory(category: string): string {
return path.join(userHome, `.${category}`, 'yarn');
}

function getCacheDirectory(): string {
function getPreferredCacheDirectories(): Array<string> {
const preferredCacheDirectories = [];

if (process.platform === 'darwin') {
return path.join(userHome, 'Library', 'Caches', 'Yarn');
preferredCacheDirectories.push(path.join(userHome, 'Library', 'Caches', 'Yarn'));
} else {
preferredCacheDirectories.push(getDirectory('cache'));
}

return getDirectory('cache');
preferredCacheDirectories.push(path.join(os.tmpdir(), '.yarn-cache'));
Copy link
Member

Choose a reason for hiding this comment

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

We should probably append a timestamp or something to the '.yarn-cache' name to make it unique across instances and runs? What do you think? (can be a follow-up)


return preferredCacheDirectories;
}

export const MODULE_CACHE_DIRECTORY = getCacheDirectory();
export const PREFERRED_MODULE_CACHE_DIRECTORIES = getPreferredCacheDirectories();
export const CONFIG_DIRECTORY = getDirectory('config');
export const LINK_REGISTRY_DIRECTORY = path.join(CONFIG_DIRECTORY, 'link');
export const GLOBAL_MODULE_DIRECTORY = path.join(CONFIG_DIRECTORY, 'global');
Expand All @@ -70,6 +77,7 @@ export const LOCKFILE_FILENAME = 'yarn.lock';
export const METADATA_FILENAME = '.yarn-metadata.json';
export const TARBALL_FILENAME = '.yarn-tarball.tgz';
export const CLEAN_FILENAME = '.yarnclean';
export const ACCESS_FILENAME = '.yarn-access';

export const DEFAULT_INDENT = ' ';
export const SINGLE_INSTANCE_PORT = 31997;
Expand Down
3 changes: 2 additions & 1 deletion src/reporters/base-reporter.js
Original file line number Diff line number Diff line change
Expand Up @@ -195,7 +195,8 @@ export default class BaseReporter {
success(message: string) {}

// a simple log message
log(message: string) {}
// TODO: rethink the {force} parameter. In the meantime, please don't use it (cf comments in #4143).
log(message: string, {force = false}: {force?: boolean} = {}) {}

// a shell command has been executed
command(command: string) {}
Expand Down
8 changes: 4 additions & 4 deletions src/reporters/console/console-reporter.js
Original file line number Diff line number Diff line change
Expand Up @@ -160,13 +160,13 @@ export default class ConsoleReporter extends BaseReporter {
this.log(this._prependEmoji(msg, '✨'));
}

log(msg: string) {
log(msg: string, {force = false}: {force?: boolean} = {}) {
this._lastCategorySize = 0;
this._log(msg);
this._log(msg, {force});
}

_log(msg: string) {
if (this.isSilent) {
_log(msg: string, {force = false}: {force?: boolean} = {}) {
if (this.isSilent && !force) {
return;
}
clearLine(this.stdout);
Expand Down
5 changes: 5 additions & 0 deletions src/reporters/lang/en.js
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,11 @@ const messages = {
workspaceNameMandatory: 'Missing name in workspace at $0, ignoring.',
workspaceNameDuplicate: 'There are more than one workspace with name $0',

cacheFolderSkipped: 'Skipping preferred cache folder $0 because it is not writable.',
cacheFolderMissing:
"Yarn hasn't been able to find a cache folder it can use. Please use the explicit --cache-folder option to tell it what location to use, or make one of the preferred locations writable.",
cacheFolderSelected: 'Selected the next writable cache folder in the list, will be $0.',

execMissingCommand: 'Missing command name.',

commandNotSpecified: 'No command specified.',
Expand Down
9 changes: 9 additions & 0 deletions src/util/fs.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,15 @@ const globModule = require('glob');
const os = require('os');
const path = require('path');

export const constants =
typeof fs.constants !== 'undefined'
Copy link
Member

Choose a reason for hiding this comment

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

Comment about Node 4 compatibility here would be great so in the future we don't get confused about this code.

? fs.constants
: {
R_OK: fs.R_OK,
W_OK: fs.W_OK,
X_OK: fs.X_OK,
};

export const lockQueue = new BlockingQueue('fs lock');

export const readFileBuffer = promisify(fs.readFile);
Expand Down
18 changes: 18 additions & 0 deletions src/util/misc.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,24 @@

const _camelCase = require('camelcase');

export function consumeStream(stream: Object): Promise<Buffer> {
return new Promise((resolve, reject) => {
const buffers = [];

stream.on(`data`, buffer => {
buffers.push(buffer);
});

stream.on(`end`, () => {
resolve(Buffer.concat(buffers));
});

stream.on(`error`, error => {
reject(error);
});
});
}

export function sortAlpha(a: string, b: string): number {
// sort alphabetically in a deterministic way
const shortLen = Math.min(a.length, b.length);
Expand Down