Skip to content

[rush] Fix install-run-rush handling of spaces in paths #1003

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

Closed
wants to merge 1 commit into from
Closed
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
16 changes: 13 additions & 3 deletions apps/rush-lib/src/scripts/install-run.ts
Original file line number Diff line number Diff line change
Expand Up @@ -352,11 +352,18 @@ function installPackage(packageInstallFolder: string, name: string, version: str

/**
* Get the ".bin" path for the package.
* If on Windows and the path contains spaces, the returned path will be quoted.
* (Paths with spaces don't seem to cause trouble for child_process.spawn() on Mac.)
*/
function getBinPath(packageInstallFolder: string, binName: string): string {
const isWindows: boolean = os.platform() === 'win32';
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;
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

const resolvedPath: string = path.resolve(binFolderPath, resolvedBinName);
if (isWindows && resolvedPath.indexOf(' ') !== -1) {
return `"${resolvedPath}"`;
}
return resolvedPath;
}

/**
Expand Down Expand Up @@ -409,7 +416,10 @@ export function installAndRun(
{
stdio: 'inherit',
cwd: process.cwd(),
env: process.env
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] === '"'
Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

}
);

Expand Down
11 changes: 11 additions & 0 deletions common/changes/@microsoft/rush/path-spaces_2018-12-20-21-57.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
{
"changes": [
{
"comment": "Fix install-run-rush handling of spaces in paths",
"packageName": "@microsoft/rush",
"type": "none"
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be patch?

Copy link
Member Author

Choose a reason for hiding this comment

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

It was automatically set to none, I'm guessing because there's already a patch update pending (Rush isn't published automatically)

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

}
],
"packageName": "@microsoft/rush",
"email": "[email protected]"
}