Fix yarn global prefix directory #3721

KishanBagaria · 2017-06-26T07:17:53Z

Previous PR: #3458

Fixes: #2064

KishanBagaria · 2017-06-26T07:20:13Z

src/cli/commands/global.js


-    return prefix;
+  try {
+    await fs.access(path.join(POSIX_GLOBAL_PREFIX, 'bin'), nativeFs.constants.W_OK);


This fs.access call can be memoized by adding the following line after it:

await config.registries.yarn.saveHomeConfig({prefix: POSIX_GLOBAL_PREFIX});

Why not use the same fs.access path for Windows too?

If you want to ensure if we have permission to write to %LOCALAPPDATA%\Yarn, that can be done. It's likely redundant though, because it's almost guaranteed to be writable, at least as writable as ~/.yarn (FALLBACK_GLOBAL_PREFIX).

I guess it won't hurt to do that.

BYK

Left some comments here: #2064 (comment)

BYK · 2017-07-03T16:46:22Z

src/cli/commands/global.js


-    return prefix;
+  try {
+    await fs.access(path.join(POSIX_GLOBAL_PREFIX, 'bin'), nativeFs.constants.W_OK);


Why not use the same fs.access path for Windows too?

BYK

Overall looks good. Just have questions about compatibility with Node and "doing the right thing" :)

BYK · 2017-07-12T16:16:51Z

src/cli/commands/global.js

    if (process.env.LOCALAPPDATA) {
-      return path.join(process.env.LOCALAPPDATA, 'Yarn', 'bin');
+      prefix = path.join(process.env.LOCALAPPDATA, 'Yarn');


Let's make this yarn as we are at it, instead of the upper-cased Yarn?

@Daniel15 @bestander any reason we were using Yarn?

@BYK The standard on Windows is to use uppercase. If you take a look in %LocalAppData% you'll see that most folders are uppercase. Some badly-behaved apps use lowercase, but it's not the norm.

Alright, NVM my comment then. Yarn should be a nice kitty.

BYK · 2017-07-12T16:26:40Z

src/constants.js

@@ -59,6 +59,9 @@ 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');

+export const POSIX_GLOBAL_PREFIX = '/usr/local';


May be we should do path.dirname('path/to/node') as suggested in https://docs.npmjs.com/files/folders#prefix-configuration ?

Perhaps we should try both. /usr/local is good if it's writable by the user, otherwise the directory that Node.js is installed from (I think there's some property on process that has the full path of the Node.js process?)

From what I understand from NPM docs, we should to this in the opposite order: try nodes folder first. That said not sure how well it would play with homebrew or my local folder hacks (I alias node to ~/Desktop/node8 etc. for quick testing purposes and I heard this is not that uncommon)

@BYK Implementing NPM's behavior of using parent directories of the node executable will come with bugs, I've described this in #3458 and #2064 (comment).

Daniel also said something similar:

The issue is that Yarn symlinks the executables into the wrong directory (the Node.js directory rather than somewhere reasonable). It doesn't really make sense to use the Node.js directory for these scripts, and on many systems that directory is not even writable by regular users.

@KishanBagaria - oh okay. Sorry I got lost in those long conversations. Thanks a lot for clarifying!

Shall we put this into the code as a comment?

No worries, the whole global-prefix thingy has been a quite long conversation indeed.

Maybe the website docs can be updated with how we get the prefix so that regular users can find the info easily, and why we don't mimic NPM.

Maybe the website docs can be updated with how we get the prefix so that regular users can find the info easily, and why we don't mimic NPM.

I think this is a great idea! @Daniel15 any suggestions? I can file an issue for this so we don't forget.

BYK · 2017-07-12T16:28:43Z

Also this patch needs a rebase to get the tests passing on CI again.

BYK

👍 for now but I'd like to talk more about https://github.com/yarnpkg/yarn/pull/3721/files#r127006212 //cc @Daniel15 @KishanBagaria

* Fix: Make sure global prefix folder is writeable when selecting it **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. * Fix wrong error message template used from getGlobalPrefix * Better error message * Add process.execPath as a last resort * Add back $DESTDIR support removed from #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

) * 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

KishanBagaria commented Jun 26, 2017

View reviewed changes

bestander requested review from BYK and arcanis July 3, 2017 16:33

BYK added this to In flight (review/code) in Yarn 1.0 Jul 5, 2017

BYK reviewed Jul 10, 2017

View reviewed changes

BYK reviewed Jul 12, 2017

View reviewed changes

BYK moved this from Awaiting Review to Active in Yarn 1.0 Jul 12, 2017

BYK self-assigned this Jul 12, 2017

Fix yarn global prefix directory

27c07db

BYK approved these changes Jul 17, 2017

View reviewed changes

BYK merged commit 2134f5b into yarnpkg:master Jul 17, 2017

BYK moved this from Active to Done in Yarn 1.0 Jul 17, 2017

KishanBagaria deleted the global-prefix-fix branch July 18, 2017 10:47

BYK mentioned this pull request Sep 6, 2017

Failing build on Heroku with 1.0.0 #4320

Closed

edmorley mentioned this pull request Sep 6, 2017

EROFS: read-only file system, access '/usr/local/bin' heroku/heroku-buildpack-nodejs#468

Closed

BYK added a commit that referenced this pull request Sep 6, 2017

Add back $DESTDIR support removed from #3721

e478af9

BYK mentioned this pull request Sep 6, 2017

Fix: More resillient global and cache folder determination #4325

Merged

BYK mentioned this pull request Sep 28, 2017

Better default value for $prefix #4574

Closed

vichle mentioned this pull request Oct 8, 2017

yarn link should create a global symlink to the binary #3639

Closed

BYK mentioned this pull request Jan 5, 2018

yarn global add doesn't install binaries properly #648

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix yarn global prefix directory #3721

Fix yarn global prefix directory #3721

KishanBagaria commented Jun 26, 2017

KishanBagaria Jun 26, 2017 •

edited

Loading

BYK Jul 3, 2017

KishanBagaria Jul 10, 2017

BYK left a comment

BYK Jul 3, 2017

BYK left a comment

BYK Jul 12, 2017

Daniel15 Jul 12, 2017

BYK Jul 12, 2017

BYK Jul 12, 2017

Daniel15 Jul 12, 2017

BYK Jul 12, 2017

KishanBagaria Jul 18, 2017

BYK Jul 18, 2017

KishanBagaria Jul 18, 2017 •

edited

Loading

BYK Jul 18, 2017

BYK commented Jul 12, 2017

BYK left a comment

Fix yarn global prefix directory #3721

Fix yarn global prefix directory #3721

Conversation

KishanBagaria commented Jun 26, 2017

KishanBagaria Jun 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BYK left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BYK left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KishanBagaria Jul 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BYK commented Jul 12, 2017

BYK left a comment

Choose a reason for hiding this comment

KishanBagaria Jun 26, 2017 •

edited

Loading

KishanBagaria Jul 18, 2017 •

edited

Loading