Fix: More resillient global and cache folder determination #4325
Conversation
**Summary** Fixes #4320 and fixes #4323. We were using `fs.access` when selecting the global prefix folder automatically which only checks for permissions but not actual writeability. This caused issues on Heroku where one of our first tries had the correct permissions but was on a read-only file system. **Test plan** Existing cache folder fallback tests should be enough for now. We should move the core of those tests for the newly added `fs.getFirstWriteableFolder` method.
src/constants.js
Outdated
| @@ -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`; | |||
src/util/fs.js
Outdated
| const result: FolderQueryResult = { | ||
| skipped: [], | ||
| folder: null, | ||
| }; |
There was a problem hiding this comment.
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.
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.
src/config.js
Outdated
| } | ||
| const cacheFolderQuery = await fs.getFirstWriteableFolder(preferredCacheFolders); | ||
| for (const failedFolder of cacheFolderQuery.skipped) { | ||
| this.reporter.warn(this.reporter.lang('cacheFolderSkipped', failedFolder)); |
There was a problem hiding this comment.
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
|
This change will increase the build size from 9.51 MB to 9.51 MB, an increase of 5.92 KB (0%)
|
BYK
changed the title
| throw err; | ||
| } | ||
| const binFolders = potentialPrefixFolders.map(prefix => path.join(prefix, 'bin')); | ||
| const prefixFolderQueryResult = await fs.getFirstSuitableFolder(binFolders); |
There was a problem hiding this comment.
Here's the problem with this code:
We only need write permissions when doing yarn global add which is usually called with sudo 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 how yarn is called (with sudo and without) which would make running yarn global add useless.
I think we should just check R_OK and X_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 that POSIX_GLOBAL_PREFIX/bin won't be writeable to yarn by default. What do you think?
|
Can v1.0.1 be published to npm? We're having the heroku issue on our build systems. |
|
@thomaswrenn should be on npm right now? |
) * Fix: Make sure global prefix folder is writeable when selecting it **Summary** Fixes yarnpkg#4320 and fixes yarnpkg#4323. We were using `fs.access` when selecting the global prefix folder automatically which only checks for permissions but not actual writeability. This caused issues on Heroku where one of our first tries had the correct permissions but was on a read-only file system. **Test plan** Existing cache folder fallback tests should be enough for now. We should move the core of those tests for the newly added `fs.getFirstWriteableFolder` method. * Fix wrong error message template used from getGlobalPrefix * Better error message * Add process.execPath as a last resort * Add back $DESTDIR support removed from yarnpkg#3721 * Fix DESTDIR typo * Fix skippedFolder error * don't use rimraf to remove a file * Don't use process.execPath * Defer write checks for global prefix * flow type * Just warn when a proper global folder cannot be found, instead of failing * Add TODO about inconsistent npm-registry code * Keep the old behavior * Update fs.js
Summary
Fixes #4320 and fixes #4323. We were using
fs.accesswhen selectingthe global prefix folder automatically and expecting only an
EACCESerror.This caused issues on Heroku where one of our first tries had threw an
EROFSerror.This patch also adds
$DESTIRsupport back, which was removed in #3721,probably inadvertently.
Test plan
Existing cache folder fallback tests should be enough for now. We should
move the core of those tests for the newly added
fs.getFirstWriteableFoldermethod.