Skip to content
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

Sdfix #45

Merged
merged 11 commits into from
Dec 15, 2023
14 changes: 14 additions & 0 deletions .github/workflows/bat.yml
Original file line number Diff line number Diff line change
Expand Up @@ -74,3 +74,17 @@ jobs:
uses: ./
with:
command: assert(onetyone==11, 'the variable `onetyone` was not set as expected by startup.m')

- run: |
mkdir subdir
echo 'onetyonetyone = 111' > subdir/startup.m
shell: bash

- name: MATLAB sd startup option is not overwritten
uses: ./
with:
command: >
assert(onetyonetyone==111);
[~, f] = fileparts(pwd);
assert(strcmp(f, 'subdir'));
startup-options: -sd subdir
9 changes: 6 additions & 3 deletions src/matlab.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,11 +33,14 @@ export async function generateScript(workspaceDir: string, command: string): Pro
const temporaryDirectory = await fs.mkdtemp(path.join(os.tmpdir(), "run_matlab_command-"));

const scriptPath = path.join(temporaryDirectory, scriptName + ".m");
await fs.writeFile(scriptPath, script.cdAndCall(workspaceDir, command), {
await fs.writeFile(scriptPath, script.cdAndCall(command), {
encoding: "utf8",
});

return { dir: temporaryDirectory, command: scriptName };
return {
dir: script.pathToCharVec(temporaryDirectory),
command: scriptName
};
}

/**
Expand All @@ -54,7 +57,7 @@ export async function runCommand(hs: HelperScript, platform: string, architectur
const rmcPath = getRunMATLABCommandScriptPath(platform, architecture);
await fs.chmod(rmcPath, 0o777);

const rmcArg = script.cdAndCall(hs.dir, hs.command);
const rmcArg = `setenv('MW_ORIG_WORKING_FOLDER', cd('${hs.dir}'));${hs.command}`;
Copy link
Member

Choose a reason for hiding this comment

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

Probably better to escape 's here right before injecting it into the char array. Then all users of this function don't need to know they need to escape the dir before calling this function and the need to escape is implementation detail of this function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wouldn't that mean that the other function consumers would likely have to escape the dir themselves later on? I can't think of many cases where someone would be calling generateScript and not wanting the directory to be escaped but i'm probably missing something

Copy link
Member

Choose a reason for hiding this comment

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

generateScript just generates a script and returns the directory that script lives in. It shouldn't assume you're then going to inject that directory path into a MATLAB char array later on. If, for instance, you wanted to delete that directory from JavaScript code, you've got an issue because the directory path is wrong.


let execArgs = [rmcArg];

Expand Down
6 changes: 3 additions & 3 deletions src/script.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright 2020 The MathWorks, Inc.
// Copyright 2020-2023 The MathWorks, Inc.

/**
* Generate MATLAB command for changing directories and calling a command in it.
Expand All @@ -7,8 +7,8 @@
* @param command Command to run in directory.
* @returns MATLAB command.
*/
export function cdAndCall(dir: string, command: string): string {
return `cd('${pathToCharVec(dir)}');${command}`;
export function cdAndCall(command: string): string {
return `cd(getenv("MW_ORIG_WORKING_FOLDER"));${command}`;
Copy link
Member

Choose a reason for hiding this comment

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

" -> '

}

/**
Expand Down
7 changes: 3 additions & 4 deletions src/script.unit.test.ts
Original file line number Diff line number Diff line change
@@ -1,15 +1,14 @@
// Copyright 2020 The MathWorks, Inc.
// Copyright 2020-2023 The MathWorks, Inc.

import * as script from "./script";

describe("call generation", () => {
it("ideally works", () => {
// I know what your thinking
const testDir = String.raw`C:\Users\you\You're Documents`;
const testCommand = "disp('hello world')";
const expectedString = String.raw`cd('C:\Users\you\You''re Documents');${testCommand}`;
const expectedString = `cd(getenv("MW_ORIG_WORKING_FOLDER"));${testCommand}`;
Copy link
Member

Choose a reason for hiding this comment

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

" -> '


expect(script.cdAndCall(testDir, testCommand)).toMatch(expectedString);
expect(script.cdAndCall(testCommand)).toMatch(expectedString);
});
});

Expand Down