Skip to content

Commit

Permalink
fix: sketch Save As preserves the folder structure
Browse files Browse the repository at this point in the history 
This commit rewrites how IDE copies sketches as part of the _Save As_
operation. Instead of copying to the destination, IDE copies the sketch
into a temporary location, then to the desired destination.

This commit drops [`cpy`](https://www.npmjs.com/package/cpy).
Ref: sindresorhus/cpy@47b89a7

Closes #2077

Signed-off-by: Akos Kitta <[email protected]>
  • Loading branch information
@kittaakos
Akos Kitta authored and kittaakos committed Jan 15, 2024
1 parent 3eef857 commit 074f654
Show file tree
Hide file tree
Showing 5 changed files with 220 additions and 140 deletions.
1 change: 0 additions & 1 deletion arduino-ide-extension/package.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,6 @@
"auth0-js": "^9.23.2",
"btoa": "^1.2.1",
"classnames": "^2.3.1",
"cpy": "^10.0.0",
"cross-fetch": "^3.1.5",
"dateformat": "^3.0.3",
"deepmerge": "^4.2.2",
Expand Down
10 changes: 10 additions & 0 deletions arduino-ide-extension/src/common/protocol/sketches-service.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ export namespace SketchesError {
export const Codes = {
NotFound: 5001,
InvalidName: 5002,
InvalidFolderName: 5003,
};
export const NotFound = ApplicationError.declare(
Codes.NotFound,
Expand All @@ -27,6 +28,15 @@ export namespace SketchesError {
};
}
);
export const InvalidFolderName = ApplicationError.declare(
Codes.InvalidFolderName,
(message: string, invalidFolderName: string) => {
return {
message,
data: { invalidFolderName },
};
}
);
}

export const SketchesServicePath = '/services/sketches-service';
Expand Down
138 changes: 97 additions & 41 deletions arduino-ide-extension/src/node/sketches-service-impl.ts
View file
Original file line number Diff line number Diff line change
@@ -1,48 +1,55 @@
import { injectable, inject, named } from '@theia/core/shared/inversify';
import { promises as fs, realpath, lstat, Stats, constants } from 'node:fs';
import os from 'node:os';
import temp from 'temp';
import path from 'node:path';
import glob from 'glob';
import crypto from 'node:crypto';
import PQueue from 'p-queue';
import { EnvVariablesServer } from '@theia/core/lib/common/env-variables';
import { ILogger } from '@theia/core/lib/common/logger';
import { nls } from '@theia/core/lib/common/nls';
import { isWindows } from '@theia/core/lib/common/os';
import { Deferred } from '@theia/core/lib/common/promise-util';
import { escapeRegExpCharacters } from '@theia/core/lib/common/strings';
import type { Mutable } from '@theia/core/lib/common/types';
import URI from '@theia/core/lib/common/uri';
import { ILogger } from '@theia/core/lib/common/logger';
import { FileUri } from '@theia/core/lib/node/file-uri';
import { ConfigServiceImpl } from './config-service-impl';
import { inject, injectable, named } from '@theia/core/shared/inversify';
import glob from 'glob';
import crypto from 'node:crypto';
import {
CopyOptions,
Stats,
constants,
promises as fs,
lstat,
realpath,
} from 'node:fs';
import os from 'node:os';
import path, { join } from 'node:path';
import PQueue from 'p-queue';
import temp from 'temp';
import { NotificationServiceServer } from '../common/protocol';
import {
SketchesService,
Sketch,
SketchRef,
SketchContainer,
SketchRef,
SketchesError,
SketchesService,
} from '../common/protocol/sketches-service';
import { NotificationServiceServer } from '../common/protocol';
import { EnvVariablesServer } from '@theia/core/lib/common/env-variables';
import { CoreClientAware } from './core-client-provider';
import {
firstToLowerCase,
firstToUpperCase,
startsWithUpperCase,
} from '../common/utils';
import {
ArchiveSketchRequest,
LoadSketchRequest,
} from './cli-protocol/cc/arduino/cli/commands/v1/commands_pb';
import { Deferred } from '@theia/core/lib/common/promise-util';
import { escapeRegExpCharacters } from '@theia/core/lib/common/strings';
import { ServiceError } from './service-error';
import { ConfigServiceImpl } from './config-service-impl';
import { CoreClientAware } from './core-client-provider';
import {
IsTempSketch,
maybeNormalizeDrive,
TempSketchPrefix,
Win32DriveRegex,
maybeNormalizeDrive,
} from './is-temp-sketch';
import { join } from 'node:path';
import { ErrnoException } from './utils/errors';
import { isWindows } from '@theia/core/lib/common/os';
import {
firstToLowerCase,
firstToUpperCase,
startsWithUpperCase,
} from '../common/utils';
import { ServiceError } from './service-error';
import { SettingsReader } from './settings-reader';
import { ErrnoException } from './utils/errors';

const RecentSketches = 'recent-sketches.json';
const DefaultIno = `void setup() {
Expand Down Expand Up @@ -510,26 +517,75 @@ export class SketchesServiceImpl
}
const sourceFolderBasename = path.basename(source);
const destinationFolderBasename = path.basename(destination);
let filter;

const errorMessage = Sketch.validateSketchFolderName(
destinationFolderBasename
);
if (errorMessage) {
const message = `${nls.localize(
'arduino/sketch/invalidSketchFolderNameMessage',
"Invalid sketch folder name: '{0}'",
destinationFolderBasename
)} ${errorMessage}`;
throw SketchesError.InvalidFolderName(message, destinationFolderBasename);
}

let filter: CopyOptions['filter'];
if (onlySketchFiles) {
const sketchFilePaths = Sketch.uris(sketch).map(FileUri.fsPath);
filter = (file: { path: string }) => sketchFilePaths.includes(file.path);
// The Windows paths, can be a trash (see below). Hence, it must be resolved with Node.js.
// After resolving the path, the drive letter is still a gamble (can be upper or lower case) and could result in a false negative match.
// Here, all sketch file paths must be resolved by Node.js, to provide the same drive letter casing.
const sketchFilePaths = await Promise.all(
Sketch.uris(sketch)
.map(FileUri.fsPath)
.map((path) => fs.realpath(path))
);
filter = async (s) => {
// On Windows, the source path could start with a complete trash. For example, \\\\?\\c:\\Users\\kittaakos\\AppData\\Local\\Temp\\.arduinoIDE-unsaved20231024-9300-1hp64fi.g8yh\\sketch_nov24d.
// The path must be resolved.
const resolvedSource = await fs.realpath(s);
if (sketchFilePaths.includes(resolvedSource)) {
return true;
}
const stat = await fs.stat(resolvedSource);
if (stat.isFile()) {
return false;
}
// Copy the folder if any of the sketch file path starts with this folder
return sketchFilePaths.some((sketchFilePath) =>
sketchFilePath.startsWith(resolvedSource)
);
};
} else {
filter = () => true;
}
const cpyModule = await import('cpy');
const cpy = cpyModule.default;
await cpy(sourceFolderBasename, destination, {
rename: (basename) =>
sourceFolderBasename !== destinationFolderBasename &&
basename === `${sourceFolderBasename}.ino`
? `${destinationFolderBasename}.ino`
: basename,

const tempRoot = await this.createTempFolder();
const temp = join(tempRoot, destinationFolderBasename);
await fs.mkdir(temp, { recursive: true });

// copy to temp folder
await fs.cp(source, temp, {
filter,
cwd: path.dirname(source),
recursive: true,
force: true,
});
const copiedSketch = await this.doLoadSketch(destinationUri, false);
return copiedSketch;

// rename the main sketch file
await fs.rename(
join(temp, `${sourceFolderBasename}.ino`),

This comment has been minimized.

Sign in to view

Copy link
@drf5n

drf5n Feb 20, 2024

This line may cause #2377
join(temp, `${destinationFolderBasename}.ino`)
);

// copy to destination
try {
await fs.cp(temp, destination, { recursive: true, force: true });
const copiedSketch = await this.doLoadSketch(destinationUri, false);
return copiedSketch;
} finally {
// remove temp
fs.rm(tempRoot, { recursive: true, force: true, maxRetries: 5 }); // no await
}
}

async archive(sketch: Sketch, destinationUri: string): Promise<string> {
Expand Down
130 changes: 109 additions & 21 deletions arduino-ide-extension/src/test/node/sketches-service-impl.slow-test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,10 @@ import { Container } from '@theia/core/shared/inversify';
import { expect } from 'chai';
import { promises as fs } from 'node:fs';
import { basename, join } from 'node:path';
import { rejects } from 'node:assert/strict';
import { sync as rimrafSync } from 'rimraf';
import temp from 'temp';
import { Sketch, SketchesService } from '../../common/protocol';
import { Sketch, SketchesError, SketchesService } from '../../common/protocol';
import {
isAccessibleSketchPath,
SketchesServiceImpl,
Expand Down Expand Up @@ -138,12 +139,31 @@ describe('sketches-service-impl', () => {

after(() => toDispose.dispose());

describe('copy', () => {
it('should copy a sketch when the destination does not exist', async function () {
this.timeout(testTimeout);
describe('copy', function () {
this.timeout(testTimeout);
this.slow(250);

it('should error when the destination sketch folder name is invalid', async () => {
const sketchesService =
container.get<SketchesServiceImpl>(SketchesService);
const tempDirPath = await sketchesService['createTempFolder']();
const destinationPath = join(tempDirPath, 'invalid with spaces');
const sketch = await sketchesService.createNewSketch();
toDispose.push(disposeSketch(sketch));
await rejects(
sketchesService.copy(sketch, {
destinationUri: FileUri.create(destinationPath).toString(),
}),
SketchesError.InvalidFolderName.is
);
});

it('should copy a sketch when the destination does not exist', async () => {
const sketchesService =
container.get<SketchesServiceImpl>(SketchesService);
const destinationPath = await sketchesService['createTempFolder']();
const tempDirPath = await sketchesService['createTempFolder']();
const destinationPath = join(tempDirPath, 'Does_Not_Exist_but_valid');
await rejects(fs.readdir(destinationPath), ErrnoException.isENOENT);
let sketch = await sketchesService.createNewSketch();
toDispose.push(disposeSketch(sketch));
const sourcePath = FileUri.fsPath(sketch.uri);
Expand Down Expand Up @@ -187,11 +207,11 @@ describe('sketches-service-impl', () => {
).to.be.true;
});

it("should copy only sketch files if 'onlySketchFiles' is true", async function () {
this.timeout(testTimeout);
it("should copy only sketch files if 'onlySketchFiles' is true", async () => {
const sketchesService =
container.get<SketchesServiceImpl>(SketchesService);
const destinationPath = await sketchesService['createTempFolder']();
const tempDirPath = await sketchesService['createTempFolder']();
const destinationPath = join(tempDirPath, 'OnlySketchFiles');
let sketch = await sketchesService.createNewSketch();
toDispose.push(disposeSketch(sketch));
const sourcePath = FileUri.fsPath(sketch.uri);
Expand All @@ -207,11 +227,25 @@ describe('sketches-service-impl', () => {
const logContent = 'log file content';
const logPath = join(sourcePath, logBasename);
await fs.writeFile(logPath, logContent, { encoding: 'utf8' });
const srcPath = join(sourcePath, 'src');
await fs.mkdir(srcPath, { recursive: true });
const libInSrcBasename = 'lib_in_src.cpp';
const libInSrcContent = 'lib in src content';
const libInSrcPath = join(srcPath, libInSrcBasename);
await fs.writeFile(libInSrcPath, libInSrcContent, { encoding: 'utf8' });
const logInSrcBasename = 'inols-clangd-err_in_src.log';
const logInSrcContent = 'log file content in src';
const logInSrcPath = join(srcPath, logInSrcBasename);
await fs.writeFile(logInSrcPath, logInSrcContent, { encoding: 'utf8' });

sketch = await sketchesService.loadSketch(sketch.uri);
expect(Sketch.isInSketch(FileUri.create(libPath), sketch)).to.be.true;
expect(Sketch.isInSketch(FileUri.create(headerPath), sketch)).to.be.true;
expect(Sketch.isInSketch(FileUri.create(logPath), sketch)).to.be.false;
expect(Sketch.isInSketch(FileUri.create(libInSrcPath), sketch)).to.be
.true;
expect(Sketch.isInSketch(FileUri.create(logInSrcPath), sketch)).to.be
.false;
const reloadedLogContent = await fs.readFile(logPath, {
encoding: 'utf8',
});
Expand Down Expand Up @@ -249,20 +283,25 @@ describe('sketches-service-impl', () => {
copied
)
).to.be.false;
try {
await fs.readFile(join(destinationPath, logBasename), {
encoding: 'utf8',
});
expect.fail(
'Log file must not exist in the destination. Expected ENOENT when loading the log file.'
);
} catch (err) {
expect(ErrnoException.isENOENT(err)).to.be.true;
}
expect(
Sketch.isInSketch(
FileUri.create(join(destinationPath, 'src', libInSrcBasename)),
copied
)
).to.be.true;
expect(
Sketch.isInSketch(
FileUri.create(join(destinationPath, 'src', logInSrcBasename)),
copied
)
).to.be.false;
await rejects(
fs.readFile(join(destinationPath, logBasename)),
ErrnoException.isENOENT
);
});

it('should copy sketch inside the sketch folder', async function () {
this.timeout(testTimeout);
it('should copy sketch inside the sketch folder', async () => {
const sketchesService =
container.get<SketchesServiceImpl>(SketchesService);
let sketch = await sketchesService.createNewSketch();
Expand Down Expand Up @@ -309,6 +348,55 @@ describe('sketches-service-impl', () => {
).to.be.true;
});

it('should not modify the subfolder structure', async () => {
const sketchesService =
container.get<SketchesServiceImpl>(SketchesService);
const tempDirPath = await sketchesService['createTempFolder']();
const destinationPath = join(tempDirPath, 'HasSubfolders_copy');
await fs.mkdir(destinationPath, { recursive: true });
let sketch = await sketchesService.createNewSketch('HasSubfolders');
toDispose.push(disposeSketch(sketch));

const sourcePath = FileUri.fsPath(sketch.uri);
const srcPath = join(sourcePath, 'src');
await fs.mkdir(srcPath, { recursive: true });
const headerPath = join(srcPath, 'FomSubfolder.h');
await fs.writeFile(headerPath, '// empty', { encoding: 'utf8' });

sketch = await sketchesService.loadSketch(sketch.uri);

expect(sketch.mainFileUri).to.be.equal(
FileUri.create(join(sourcePath, 'HasSubfolders.ino')).toString()
);
expect(sketch.additionalFileUris).to.be.deep.equal([
FileUri.create(join(srcPath, 'FomSubfolder.h')).toString(),
]);
expect(sketch.otherSketchFileUris).to.be.empty;
expect(sketch.rootFolderFileUris).to.be.empty;

const destinationUri = FileUri.create(destinationPath).toString();
const copySketch = await sketchesService.copy(sketch, { destinationUri });
toDispose.push(disposeSketch(copySketch));
expect(copySketch.mainFileUri).to.be.equal(
FileUri.create(
join(destinationPath, 'HasSubfolders_copy.ino')
).toString()
);
expect(copySketch.additionalFileUris).to.be.deep.equal([
FileUri.create(
join(destinationPath, 'src', 'FomSubfolder.h')
).toString(),
]);
expect(copySketch.otherSketchFileUris).to.be.empty;
expect(copySketch.rootFolderFileUris).to.be.empty;

const actualHeaderContent = await fs.readFile(
join(destinationPath, 'src', 'FomSubfolder.h'),
{ encoding: 'utf8' }
);
expect(actualHeaderContent).to.be.equal('// empty');
});

it('should copy sketch with overwrite when source and destination sketch folder names are the same', async function () {
this.timeout(testTimeout);
const sketchesService =
Expand Down Expand Up @@ -346,7 +434,7 @@ describe('sketches-service-impl', () => {
[
'<',
'>',
'chevrons',
'lt+gt',
{
predicate: () => isWindows,
why: '< (less than) and > (greater than) are reserved characters on Windows (https://learn.microsoft.com/en-us/windows/win32/fileio/naming-a-file#naming-conventions)',
Expand Down
Loading

0 comments on commit 074f654

Please sign in to comment.