[rush] Fix install-run-rush handling of spaces in paths#1003
[rush] Fix install-run-rush handling of spaces in paths#1003ecraig12345 wants to merge 1 commit into
Conversation
| { | ||
| "comment": "Fix install-run-rush handling of spaces in paths", | ||
| "packageName": "@microsoft/rush", | ||
| "type": "none" |
There was a problem hiding this comment.
It was automatically set to none, I'm guessing because there's already a patch update pending (Rush isn't published automatically)
There was a problem hiding this comment.
Rush uses lockstep versioning, so the version bump is determined by the config file that governs the next release, not by the person running rush change. I agree that "type": "none" doesn't convey this very clearly.
| const binFolderPath: string = path.resolve(packageInstallFolder, NODE_MODULES_FOLDER_NAME, '.bin'); | ||
| const resolvedBinName: string = (os.platform() === 'win32') ? `${binName}.cmd` : binName; | ||
| return path.resolve(binFolderPath, resolvedBinName); | ||
| const resolvedBinName: string = isWindows ? `${binName}.cmd` : binName; |
There was a problem hiding this comment.
getBinPath() is supposed to return a path. A path should not contain shell escaping. The quotation marks you are adding here are an encoding detail of a particular shell, so this isn't a good place to apply that logic.
| env: process.env, | ||
| // If binPath contains spaces, it must be quoted, and quoted paths will only be processed | ||
| // correctly if we use a shell (otherwise don't use a shell since it's a bit less efficient). | ||
| shell: binPath[0] === '"' |
There was a problem hiding this comment.
I'm not sure there is an easy general solution here. See these notes for some background.
But what happens if we simply set shell: true in all cases? Or always when the OS is Windows? Would that improve the situation you encountered without breaking other scenarios?
|
Hmm so this is why some programs/packages just say don’t use spaces...what a minefield...
I don’t actually know what problems using a shell might create (if any), which is why I did it only when necessary. Supposedly it’s a bit less efficient but I doubt that matters much here.
|
octogonz
changed the title
5 participants
On Windows, if the user puts their Git repo in a path with spaces (as in microsoft/fluentui#7508) and tries to use the
install-run-rush.jsscript, it will fail. For whatever reason, paths with spaces work fine on Macs.Fix is to wrap the path in quotes if it contains any spaces, then spawn the child process in a shell so the quotes are interpreted properly (as suggested by nodejs/node#7367 (comment)). My current implementation only uses quotes and a shell when necessary, but it could be changed to always do so.