diff --git a/news/2 Fixes/5386.md b/news/2 Fixes/5386.md new file mode 100644 index 000000000000..ab80b86d964e --- /dev/null +++ b/news/2 Fixes/5386.md @@ -0,0 +1 @@ +Fix import/export paths to be escaped on windows. \ No newline at end of file diff --git a/package-lock.json b/package-lock.json index f195b0086a16..a3ecd5d820e6 100644 --- a/package-lock.json +++ b/package-lock.json @@ -3959,14 +3959,12 @@ "balanced-match": { "version": "1.0.0", "bundled": true, - "dev": true, - "optional": true + "dev": true }, "brace-expansion": { "version": "1.1.11", "bundled": true, "dev": true, - "optional": true, "requires": { "balanced-match": "^1.0.0", "concat-map": "0.0.1" @@ -3981,20 +3979,17 @@ "code-point-at": { "version": "1.1.0", "bundled": true, - "dev": true, - "optional": true + "dev": true }, "concat-map": { "version": "0.0.1", "bundled": true, - "dev": true, - "optional": true + "dev": true }, "console-control-strings": { "version": "1.1.0", "bundled": true, - "dev": true, - "optional": true + "dev": true }, "core-util-is": { "version": "1.0.2", @@ -4111,8 +4106,7 @@ "inherits": { "version": "2.0.3", "bundled": true, - "dev": true, - "optional": true + "dev": true }, "ini": { "version": "1.3.5", @@ -4124,7 +4118,6 @@ "version": "1.0.0", "bundled": true, "dev": true, - "optional": true, "requires": { "number-is-nan": "^1.0.0" } @@ -4139,7 +4132,6 @@ "version": "3.0.4", "bundled": true, "dev": true, - "optional": true, "requires": { "brace-expansion": "^1.1.7" } @@ -4147,14 +4139,12 @@ "minimist": { "version": "0.0.8", "bundled": true, - "dev": true, - "optional": true + "dev": true }, "minipass": { "version": "2.3.5", "bundled": true, "dev": true, - "optional": true, "requires": { "safe-buffer": "^5.1.2", "yallist": "^3.0.0" @@ -4173,7 +4163,6 @@ "version": "0.5.1", "bundled": true, "dev": true, - "optional": true, "requires": { "minimist": "0.0.8" } @@ -4254,8 +4243,7 @@ "number-is-nan": { "version": "1.0.1", "bundled": true, - "dev": true, - "optional": true + "dev": true }, "object-assign": { "version": "4.1.1", @@ -4267,7 +4255,6 @@ "version": "1.4.0", "bundled": true, "dev": true, - "optional": true, "requires": { "wrappy": "1" } @@ -4389,7 +4376,6 @@ "version": "1.0.2", "bundled": true, "dev": true, - "optional": true, "requires": { "code-point-at": "^1.0.0", "is-fullwidth-code-point": "^1.0.0", @@ -7481,14 +7467,12 @@ "balanced-match": { "version": "1.0.0", "bundled": true, - "dev": true, - "optional": true + "dev": true }, "brace-expansion": { "version": "1.1.11", "bundled": true, "dev": true, - "optional": true, "requires": { "balanced-match": "^1.0.0", "concat-map": "0.0.1" @@ -7508,8 +7492,7 @@ "concat-map": { "version": "0.0.1", "bundled": true, - "dev": true, - "optional": true + "dev": true }, "console-control-strings": { "version": "1.1.0", @@ -7657,7 +7640,6 @@ "version": "3.0.4", "bundled": true, "dev": true, - "optional": true, "requires": { "brace-expansion": "^1.1.7" } @@ -12365,7 +12347,7 @@ "dependencies": { "convert-source-map": { "version": "1.6.0", - "resolved": false, + "resolved": "https://registry.npmjs.org/convert-source-map/-/convert-source-map-1.6.0.tgz", "integrity": "sha512-eFu7XigvxdZ1ETfbgPBohgyQ/Z++C0eEhTor0qRwBw9unw+L0/6V8wkSuGgzdThkiS5lSpdptOQPD8Ak40a+7A==", "dev": true, "requires": { @@ -12374,7 +12356,7 @@ }, "execa": { "version": "1.0.0", - "resolved": false, + "resolved": "https://registry.npmjs.org/execa/-/execa-1.0.0.tgz", "integrity": "sha512-adbxcyWV46qiHyvSp50TKt05tB4tK3HcmF7/nxfAdhnox83seTDbwnaqKO4sXRy7roHAIFqJP/Rw/AuEbX61LA==", "dev": true, "requires": { @@ -12400,7 +12382,7 @@ }, "find-up": { "version": "3.0.0", - "resolved": false, + "resolved": "https://registry.npmjs.org/find-up/-/find-up-3.0.0.tgz", "integrity": "sha512-1yD6RmLI1XBfxugvORwlck6f75tYL+iR0jqwsOrOxMZyGYqUuDhJ0l4AXdO1iX/FTs9cBAMEk1gWSEx1kSbylg==", "dev": true, "requires": { @@ -12415,7 +12397,7 @@ }, "get-stream": { "version": "4.1.0", - "resolved": false, + "resolved": "https://registry.npmjs.org/get-stream/-/get-stream-4.1.0.tgz", "integrity": "sha512-GMat4EJ5161kIy2HevLlr4luNjBgvmj413KaQA7jt4V8B4RDsfpHk7WQ9GVqfYyyx8OS/L66Kox+rJRNklLK7w==", "dev": true, "requires": { @@ -12424,7 +12406,7 @@ }, "glob": { "version": "7.1.3", - "resolved": false, + "resolved": "https://registry.npmjs.org/glob/-/glob-7.1.3.tgz", "integrity": "sha512-vcfuiIxogLV4DlGBHIUOwI0IbrJ8HWPc4MU7HzviGeNho/UJDfi6B5p3sHeWIQ0KGIU0Jpxi5ZHxemQfLkkAwQ==", "dev": true, "requires": { @@ -12438,7 +12420,7 @@ }, "locate-path": { "version": "3.0.0", - "resolved": false, + "resolved": "https://registry.npmjs.org/locate-path/-/locate-path-3.0.0.tgz", "integrity": "sha512-7AO748wWnIhNqAuaty2ZWHkQHRSNfPVIsPIfwEOWO22AmaoVrWavlOcMR5nzTLNYvp36X220/maaRsrec1G65A==", "dev": true, "requires": { @@ -12458,7 +12440,7 @@ }, "os-locale": { "version": "3.1.0", - "resolved": false, + "resolved": "https://registry.npmjs.org/os-locale/-/os-locale-3.1.0.tgz", "integrity": "sha512-Z8l3R4wYWM40/52Z+S265okfFj8Kt2cC2MKY+xNi3kFs+XGI7WXu/I309QQQYbRW4ijiZ+yxs9pqEhJh0DqW3Q==", "dev": true, "requires": { @@ -12469,7 +12451,7 @@ }, "path-exists": { "version": "3.0.0", - "resolved": false, + "resolved": "https://registry.npmjs.org/path-exists/-/path-exists-3.0.0.tgz", "integrity": "sha1-zg6+ql94yxiSXqfYENe1mwEP1RU=", "dev": true }, @@ -12481,7 +12463,7 @@ }, "pkg-dir": { "version": "3.0.0", - "resolved": false, + "resolved": "https://registry.npmjs.org/pkg-dir/-/pkg-dir-3.0.0.tgz", "integrity": "sha512-/E57AYkoeQ25qkxMj5PBOVgF8Kiu/h7cYS30Z5+R7WaiCCBfLq58ZI/dSeaEKb9WVJV5n/03QwrN3IeWIFllvw==", "dev": true, "requires": { @@ -12490,7 +12472,7 @@ }, "pump": { "version": "3.0.0", - "resolved": false, + "resolved": "https://registry.npmjs.org/pump/-/pump-3.0.0.tgz", "integrity": "sha512-LwZy+p3SFs1Pytd/jYct4wpv49HiYCqd9Rlc5ZVdk0V+8Yzv6jR5Blk3TRmPL1ft69TxP0IMZGJ+WPFU2BFhww==", "dev": true, "requires": { @@ -12506,13 +12488,13 @@ }, "resolve-from": { "version": "4.0.0", - "resolved": false, + "resolved": "https://registry.npmjs.org/resolve-from/-/resolve-from-4.0.0.tgz", "integrity": "sha512-pb/MYmXstAkysRFx8piNI1tGFNQIFA3vkE3Gq4EuA1dF6gHp/+vgZqsCGJapvy8N3Q+4o7FwvquPJcnZ7RYy4g==", "dev": true }, "rimraf": { "version": "2.6.3", - "resolved": false, + "resolved": "https://registry.npmjs.org/rimraf/-/rimraf-2.6.3.tgz", "integrity": "sha512-mwqeW5XsA2qAejG46gYdENaxXjx9onRNCfn7L0duuP4hCuTIi/QO7PDK07KJfp1d+izWPrzEJDcSqBa0OZQriA==", "dev": true, "requires": { diff --git a/src/client/datascience/constants.ts b/src/client/datascience/constants.ts index 3eedc81d0659..7c1f0f398881 100644 --- a/src/client/datascience/constants.ts +++ b/src/client/datascience/constants.ts @@ -122,7 +122,8 @@ export namespace Settings { } export namespace CodeSnippits { - export const ChangeDirectory = ['{0}', 'import os', 'try:', '\tos.chdir(os.path.join(os.getcwd(), \'{1}\'))', '\tprint(os.getcwd())', 'except:', '\tpass', '']; + export const ChangeDirectory = ['{0}', '{1}', 'import os', 'try:', '\tos.chdir(os.path.join(os.getcwd(), \'{2}\'))', '\tprint(os.getcwd())', 'except:', '\tpass', '']; + export const ChangeDirectoryCommentIdentifier = '# ms-python.python added'; // Not translated so can compare. } export namespace Identifiers { diff --git a/src/client/datascience/jupyter/jupyterExporter.ts b/src/client/datascience/jupyter/jupyterExporter.ts index 72ce08bb7037..94c91a0d50f0 100644 --- a/src/client/datascience/jupyter/jupyterExporter.ts +++ b/src/client/datascience/jupyter/jupyterExporter.ts @@ -8,11 +8,12 @@ import * as path from 'path'; import * as uuid from 'uuid/v4'; import { IWorkspaceService } from '../../common/application/types'; -import { IFileSystem } from '../../common/platform/types'; +import { IFileSystem, IPlatformService } from '../../common/platform/types'; import { IConfigurationService, ILogger } from '../../common/types'; import * as localize from '../../common/utils/localize'; import { noop } from '../../common/utils/misc'; import { CellMatcher } from '../cellMatcher'; +import { concatMultilineString } from '../common'; import { CodeSnippits, Identifiers } from '../constants'; import { CellState, ICell, IJupyterExecution, INotebookExporter, ISysInfo } from '../types'; @@ -24,7 +25,9 @@ export class JupyterExporter implements INotebookExporter { @inject(ILogger) private logger: ILogger, @inject(IWorkspaceService) private workspaceService: IWorkspaceService, @inject(IConfigurationService) private configService: IConfigurationService, - @inject(IFileSystem) private fileSystem: IFileSystem) { + @inject(IFileSystem) private fileSystem: IFileSystem, + @inject(IPlatformService) private readonly platform: IPlatformService + ) { } public dispose() { @@ -75,7 +78,7 @@ export class JupyterExporter implements INotebookExporter { const changeDirectory = await this.calculateDirectoryChange(file, cells); if (changeDirectory) { - const exportChangeDirectory = CodeSnippits.ChangeDirectory.join(os.EOL).format(localize.DataScience.exportChangeDirectoryComment(), changeDirectory); + const exportChangeDirectory = CodeSnippits.ChangeDirectory.join(os.EOL).format(localize.DataScience.exportChangeDirectoryComment(), CodeSnippits.ChangeDirectoryCommentIdentifier, changeDirectory); const cell: ICell = { data: { @@ -117,21 +120,30 @@ export class JupyterExporter implements INotebookExporter { } private calculateDirectoryChange = async (notebookFile: string, cells: ICell[]): Promise => { + // Make sure we don't already have a cell with a ChangeDirectory comment in it. let directoryChange: string | undefined; - const notebookFilePath = path.dirname(notebookFile); - // First see if we have a workspace open, this only works if we have a workspace root to be relative to - if (this.workspaceService.hasWorkspaceFolders) { - const workspacePath = await this.firstWorkspaceFolder(cells); - - // Make sure that we have everything that we need here - if (workspacePath && path.isAbsolute(workspacePath) && notebookFilePath && path.isAbsolute(notebookFilePath)) { - directoryChange = path.relative(notebookFilePath, workspacePath); + const haveChangeAlready = cells.find(c => concatMultilineString(c.data.source).includes(CodeSnippits.ChangeDirectoryCommentIdentifier)); + if (!haveChangeAlready) { + const notebookFilePath = path.dirname(notebookFile); + // First see if we have a workspace open, this only works if we have a workspace root to be relative to + if (this.workspaceService.hasWorkspaceFolders) { + const workspacePath = await this.firstWorkspaceFolder(cells); + + // Make sure that we have everything that we need here + if (workspacePath && path.isAbsolute(workspacePath) && notebookFilePath && path.isAbsolute(notebookFilePath)) { + directoryChange = path.relative(notebookFilePath, workspacePath); + } } } // If path.relative can't calculate a relative path, then it just returns the full second path // so check here, we only want this if we were able to calculate a relative path, no network shares or drives if (directoryChange && !path.isAbsolute(directoryChange)) { + // Escape windows path chars so they end up in the source escaped + if (this.platform.isWindows) { + directoryChange = directoryChange.replace('\\', '\\\\'); + } + return directoryChange; } else { return undefined; diff --git a/src/client/datascience/jupyter/jupyterImporter.ts b/src/client/datascience/jupyter/jupyterImporter.ts index 718c60773fb7..3f2a4015d6c3 100644 --- a/src/client/datascience/jupyter/jupyterImporter.ts +++ b/src/client/datascience/jupyter/jupyterImporter.ts @@ -7,7 +7,7 @@ import * as os from 'os'; import * as path from 'path'; import { IWorkspaceService } from '../../common/application/types'; -import { IFileSystem } from '../../common/platform/types'; +import { IFileSystem, IPlatformService } from '../../common/platform/types'; import { IConfigurationService, IDisposableRegistry } from '../../common/types'; import * as localize from '../../common/utils/localize'; import { noop } from '../../common/utils/misc'; @@ -38,7 +38,9 @@ export class JupyterImporter implements INotebookImporter { @inject(IDisposableRegistry) private disposableRegistry: IDisposableRegistry, @inject(IConfigurationService) private configuration: IConfigurationService, @inject(IJupyterExecution) private jupyterExecution: IJupyterExecution, - @inject(IWorkspaceService) private workspaceService: IWorkspaceService) { + @inject(IWorkspaceService) private workspaceService: IWorkspaceService, + @inject(IPlatformService) private readonly platform: IPlatformService + ) { this.templatePromise = this.createTemplateFile(); } @@ -49,7 +51,7 @@ export class JupyterImporter implements INotebookImporter { const settings = this.configuration.getSettings(); let directoryChange: string | undefined; if (settings.datascience.changeDirOnImportExport) { - directoryChange = this.calculateDirectoryChange(file); + directoryChange = await this.calculateDirectoryChange(file); } // Use the jupyter nbconvert functionality to turn the notebook into a python file @@ -70,27 +72,39 @@ export class JupyterImporter implements INotebookImporter { } private addDirectoryChange = (pythonOutput: string, directoryChange: string): string => { - const newCode = CodeSnippits.ChangeDirectory.join(os.EOL).format(localize.DataScience.importChangeDirectoryComment(), directoryChange); + const newCode = CodeSnippits.ChangeDirectory.join(os.EOL).format(localize.DataScience.importChangeDirectoryComment(), CodeSnippits.ChangeDirectoryCommentIdentifier, directoryChange); return newCode.concat(pythonOutput); } // When importing a file, calculate if we can create a %cd so that the relative paths work - private calculateDirectoryChange = (notebookFile: string): string | undefined => { + private async calculateDirectoryChange(notebookFile: string): Promise { let directoryChange: string | undefined; - const notebookFilePath = path.dirname(notebookFile); - // First see if we have a workspace open, this only works if we have a workspace root to be relative to - if (this.workspaceService.hasWorkspaceFolders) { - const workspacePath = this.workspaceService.workspaceFolders![0].uri.fsPath; - - // Make sure that we have everything that we need here - if (workspacePath && path.isAbsolute(workspacePath) && notebookFilePath && path.isAbsolute(notebookFilePath)) { - directoryChange = path.relative(workspacePath, notebookFilePath); + // Make sure we don't already have an import/export comment in the file + const contents = await this.fileSystem.readFile(notebookFile); + const haveChangeAlready = contents.includes(CodeSnippits.ChangeDirectoryCommentIdentifier); + + if (!haveChangeAlready) { + const notebookFilePath = path.dirname(notebookFile); + // First see if we have a workspace open, this only works if we have a workspace root to be relative to + if (this.workspaceService.hasWorkspaceFolders) { + const workspacePath = this.workspaceService.workspaceFolders![0].uri.fsPath; + + // Make sure that we have everything that we need here + if (workspacePath && path.isAbsolute(workspacePath) && notebookFilePath && path.isAbsolute(notebookFilePath)) { + directoryChange = path.relative(workspacePath, notebookFilePath); + } } } // If path.relative can't calculate a relative path, then it just returns the full second path // so check here, we only want this if we were able to calculate a relative path, no network shares or drives if (directoryChange && !path.isAbsolute(directoryChange)) { + + // Escape windows path chars so they end up in the source escaped + if (this.platform.isWindows) { + directoryChange = directoryChange.replace('\\', '\\\\'); + } + return directoryChange; } else { return undefined; diff --git a/src/test/datascience/notebook.functional.test.ts b/src/test/datascience/notebook.functional.test.ts index 97c3710999b2..ccf3daa77664 100644 --- a/src/test/datascience/notebook.functional.test.ts +++ b/src/test/datascience/notebook.functional.test.ts @@ -354,8 +354,11 @@ suite('DataScience notebook tests', () => { // Try importing this. This should verify export works and that importing is possible const results = await importer.importFromFile(temp.filePath); - // Make sure we added a chdir into our results - assert.ok(results.includes('os.chdir')); + // Make sure we have a single chdir in our results + const first = results.indexOf('os.chdir'); + assert.ok(first >= 0, 'No os.chdir in import'); + const second = results.indexOf('os.chdir', first + 1); + assert.equal(second, -1, 'More than one chdir in the import. It should be skipped'); // Make sure we have a cell in our results assert.ok(/#\s*%%/.test(results), 'No cells in returned import');