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

Fix cross-device mv by switching to copy #846

Closed
wants to merge 33 commits into from
Closed
Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
53116ae
sm-polyfilling
seanmorris Dec 6, 2023
e985f2e
Updating mv to work across devices
seanmorris Dec 6, 2023
25471e2
Refactoring.
seanmorris Dec 6, 2023
aa23eb5
Tweaks.
seanmorris Dec 6, 2023
fe314c0
Separating polyfills.
seanmorris Dec 12, 2023
71b5108
Separating polyfills.
seanmorris Dec 12, 2023
b1dd043
Switching from move to copy.
seanmorris Dec 12, 2023
430cd8a
Switching from move to copy.
seanmorris Dec 12, 2023
0575d89
Lint
seanmorris Dec 12, 2023
a45592e
Comment.
seanmorris Dec 13, 2023
c774621
More readable copy function
seanmorris Dec 15, 2023
fd8f768
Simplifying.
seanmorris Dec 15, 2023
6d8c576
Comments.
seanmorris Dec 15, 2023
06c2315
Guard clause.
seanmorris Dec 15, 2023
b760a1d
Unit test for copy, extending timeout for networking test
seanmorris Dec 15, 2023
d112c05
Explicit recursive copy.
seanmorris Dec 15, 2023
5097fb1
Adding directory copy test.
seanmorris Dec 15, 2023
b182c56
Expanding timeout for e2e
seanmorris Dec 15, 2023
396874a
Expanding timeout for e2e
seanmorris Dec 15, 2023
21b86e2
Extending test timeout.
seanmorris Dec 15, 2023
d9d8201
Fixing copy for web
seanmorris Dec 16, 2023
25b50f8
Fixing copy for web
seanmorris Dec 16, 2023
9e8453b
Reverting.
seanmorris Dec 16, 2023
cd3902f
PR comments.
seanmorris Dec 16, 2023
0a3ef1e
Options object.
seanmorris Dec 16, 2023
577d824
options object
seanmorris Dec 16, 2023
100c4db
options object
seanmorris Dec 16, 2023
ca027d4
Typescript fix.
seanmorris Dec 16, 2023
6f97978
Typescript fix.
seanmorris Dec 16, 2023
36c17b8
Unit test
seanmorris Dec 16, 2023
bcb225d
More readablity.
seanmorris Dec 16, 2023
8da6708
File to Dir test.
seanmorris Dec 16, 2023
cc576fc
doc comment
seanmorris Dec 16, 2023
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
2 changes: 1 addition & 1 deletion packages/php-wasm/node/src/test/php-networking.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ describe.each(SupportedPHPVersions)(
expect(text).toEqual('response from express');
});
},
1000
5000
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why change this value to 5000?

Copy link
Contributor Author

@seanmorris seanmorris Dec 16, 2023

Choose a reason for hiding this comment

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

@adamziel This change was originally related to the e2e crash, which I suspected to be resource related, but I quickly determined that this wasn't the cause once I was able to get things running locally, so I've reverted this change.

);

async function startServer() {
Expand Down
67 changes: 63 additions & 4 deletions packages/php-wasm/node/src/test/php.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ describe.each(SupportedPHPVersions)('PHP %s', (phpVersion) => {
$pipes
);

// Yields back to JS event loop to capture and process the
// Yields back to JS event loop to capture and process the
// child_process output. This is fine. Regular PHP scripts
// typically wait for the child process to finish.
sleep(1);
Expand Down Expand Up @@ -122,7 +122,7 @@ describe.each(SupportedPHPVersions)('PHP %s', (phpVersion) => {
// This test fails
if (!['7.0', '7.1', '7.2', '7.3'].includes(phpVersion)) {
/*
There is a race condition in this variant of the test which
There is a race condition in this variant of the test which
causes the following failure (but only sometimes):

src/test/php.spec.ts > PHP 8.2 > proc_open() > cat – stdin=pipe, stdout=file, stderr=file
Expand All @@ -142,7 +142,7 @@ describe.each(SupportedPHPVersions)('PHP %s', (phpVersion) => {
);
fwrite($pipes[0], 'WordPress\n');

// Yields back to JS event loop to capture and process the
// Yields back to JS event loop to capture and process the
// child_process output. This is fine. Regular PHP scripts
// typically wait for the child process to finish.
sleep(1);
Expand Down Expand Up @@ -173,7 +173,7 @@ describe.each(SupportedPHPVersions)('PHP %s', (phpVersion) => {
$pipes
);

// Yields back to JS event loop to capture and process the
// Yields back to JS event loop to capture and process the
// child_process output. This is fine. Regular PHP scripts
// typically wait for the child process to finish.
sleep(1);
Expand Down Expand Up @@ -324,6 +324,65 @@ describe.each(SupportedPHPVersions)('PHP %s', (phpVersion) => {
);
});

it('cp() should copy a file', () => {
php.mkdir(testDirPath);
const file1 = testDirPath + '/1.txt';
const file2 = testDirPath + '/2.txt';
php.writeFile(file1, '1');
php.cp(file1, file2);
expect(php.fileExists(file1)).toEqual(true);
expect(php.fileExists(file2)).toEqual(true);
expect(php.readFileAsText(file2)).toEqual('1');
});

it('cp() should copy a directory', () => {
const testDirPath1 = testDirPath;
const testDirPath2 = '/__test987654322';
php.mkdir(testDirPath1);
php.writeFile(testDirPath1 + '/1.txt', '1');
php.writeFile(testDirPath1 + '/2.txt', '2');
php.cp(testDirPath1, testDirPath2, true);
expect(php.fileExists(testDirPath2 + '/1.txt')).toEqual(true);
expect(php.fileExists(testDirPath2 + '/2.txt')).toEqual(true);
expect(php.readFileAsText(testDirPath2 + '/1.txt')).toEqual('1');
expect(php.readFileAsText(testDirPath2 + '/2.txt')).toEqual('2');
});

it('cp() should replace target file if it exists', () => {
php.mkdir(testDirPath);
const file1 = testDirPath + '/1.txt';
const file2 = testDirPath + '/2.txt';
php.writeFile(file1, '1');
php.writeFile(file2, '2');
php.cp(file1, file2);
expect(php.fileExists(file1)).toEqual(true);
expect(php.fileExists(file2)).toEqual(true);
expect(php.readFileAsText(file2)).toEqual('1');
});

it('cp() should throw a useful error when source file does not exist', () => {
php.mkdir(testDirPath);
const file1 = testDirPath + '/1.txt';
const file2 = testDirPath + '/2.txt';
expect(() => {
php.cp(file1, file2);
}).toThrowError(
`Could not copy "${testDirPath}/1.txt": There is no such file or directory OR the parent directory does not exist.`
);
});

it('cp() should throw a useful error when target directory does not exist', () => {
php.mkdir(testDirPath);
const file1 = testDirPath + '/1.txt';
const file2 = testDirPath + '/nowhere/2.txt';
php.writeFile(file1, '1');
expect(() => {
php.cp(file1, file2);
}).toThrowError(
`Could not copy "${testDirPath}/1.txt": There is no such file or directory OR the parent directory does not exist.`
);
});

it('mkdir() should create a directory', () => {
php.mkdir(testDirPath);
expect(php.fileExists(testDirPath)).toEqual(true);
Expand Down
49 changes: 48 additions & 1 deletion packages/php-wasm/universal/src/lib/base-php.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ import {
improveWASMErrorReporting,
UnhandledRejectionsTarget,
} from './wasm-error-reporting';
import { Semaphore } from '@php-wasm/util';
import { Semaphore, joinPaths } from '@php-wasm/util';

const STRING = 'string';
const NUMBER = 'number';
Expand Down Expand Up @@ -616,6 +616,53 @@ export abstract class BasePHP implements IsomorphicLocalPHP {
this[__private__dont__use].FS.rename(fromPath, toPath);
}

/** @inheritDoc */
@rethrowFileSystemError('Could not copy "{path}"')
cp(fromPath: string, toPath: string, recursive = false) {
const FS = this[__private__dont__use].FS;

const fromStat = FS.stat(fromPath);

// Emscripten directories will have the sticky-bit set
// https://linux.die.net/man/1/chmod
const fromIsDir = fromStat.mode & 0o40000;

let toExists: boolean;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if this would be more readable, what do you think?

Suggested change
let toExists: boolean;
let destinationExists: boolean;


try {
FS.stat(toPath);
toExists = true;
} catch {
toExists = false;
}

if (!fromIsDir) {
const file = FS.readFile(fromPath, { encoding: 'binary' });
Copy link
Collaborator

@adamziel adamziel Dec 15, 2023

Choose a reason for hiding this comment

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

Isn't { encoding: 'binary' } the default? Also, let's use functions available on this whenever possible, e.g. there's this.readFileAsBuffer()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

const file = this.readFileAsBuffer(sourcePath);

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks! What about other places where this functions could be applicable over custom-built logic?

FS.writeFile(toPath, file);
Copy link
Collaborator

Choose a reason for hiding this comment

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

What should happen if toPath exists and is a directory?

Copy link
Contributor Author

@seanmorris seanmorris Dec 16, 2023

Choose a reason for hiding this comment

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

@adam In that case, it('cp() should copy a file into a directory', () => {...

if (destinationIsDir) {
FS.writeFile(
joinPaths(destinationPath, basename(sourcePath)),
file
);
} else {
FS.writeFile(destinationPath, file);
}

it('cp() should copy a file into a directory', () => {
const testDirPath1 = testDirPath;
const testDirPath2 = '/__test987654322';
php.mkdir(testDirPath1);
php.mkdir(testDirPath2);
php.writeFile(testDirPath1 + '/1.txt', '1');
php.cp(testDirPath1 + '/1.txt', testDirPath2);
expect(php.fileExists(testDirPath2 + '/1.txt')).toEqual(true);
expect(php.readFileAsText(testDirPath2 + '/1.txt')).toEqual('1');
});

Copy link
Collaborator

@adamziel adamziel Dec 18, 2023

Choose a reason for hiding this comment

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

That test only covers a case when oldPath is a file, right? There's also a scenario when it's a directory and that also needs a test.

return;
}

if (!recursive) {
throw new Error(
`Cannot use non-recurive copy on directory: ${fromPath}`
);
}

if (!toExists) {
FS.mkdir(toPath);
}

const files = this.listFiles(fromPath);

files.forEach((file: string) =>
this.cp(
joinPaths(fromPath, file),
joinPaths(toPath, file),
recursive
)
);
}

/** @inheritDoc */
@rethrowFileSystemError('Could not remove directory "{path}"')
rmdir(path: string, options: RmDirOptions = { recursive: true }) {
Expand Down
9 changes: 9 additions & 0 deletions packages/php-wasm/universal/src/lib/universal-php.ts
Original file line number Diff line number Diff line change
Expand Up @@ -256,6 +256,15 @@ export interface IsomorphicLocalPHP extends RequestHandler {
*/
mv(oldPath: string, newPath: string): void;

/**
* Copies a file or directory in the PHP filesystem to a
* new location.
Copy link
Collaborator

@adamziel adamziel Dec 15, 2023

Choose a reason for hiding this comment

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

Let's thoroughly explain what this function does in different situations, like when the target path already exists.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

/**
* Copies a file or directory in the PHP filesystem to a
* new location. The target directory's parent must be a
* valid location in the filesystem prior to copying.
*
* If the target path is a file that already exists,
* it will be overwritten.
*
* If the target path is a directory that already exists,
* the file or directory will be copied into it.
*
* If the target path's parent directory does not exist,
* an error will be thrown.
*
* @param oldPath The file or directory to be copied.
* @param newPath The new, full path to copy the file or directory to.
*/

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thank you! As I'm read it, I have additional questions:

The target directory's parent must be a valid location

A valid location means only that it must exist, right? If yes, let's say it directly.

If the target path is a file that already exists, it will be overwritten.

Is that the case only when the source path is a file? Or also when it's a directory?

If the target path is a directory that already exists, the file or directory will be copied into it.

So if the target path is a directory that does not exist yet, will it be created?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I also wonder about the semantics here. Right now this function makes an implicit assumption that says:

If the newPath is a directory that already exists, the developer intended to copy the oldPath inside that directory.

I don't think it always holds. Maybe the newPath shouldn't actually be there? I'd rather remove the implicit assumption and make the logic explicit: If newPath is a directory that already exists, that's an error, please remove it explicitly before the recursive copy.

*
* @param oldPath The file or directory to be copied.
* @param newPath The new, full path to copy the file or directory to.
*/
cp(oldPath: string, newPath: string, recurive: boolean): void;

/**
* Removes a directory from the PHP filesystem.
*
Expand Down
5 changes: 5 additions & 0 deletions packages/php-wasm/web/src/lib/web-php-endpoint.ts
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,11 @@ export class WebPHPEndpoint implements IsomorphicLocalPHP {
return _private.get(this)!.php.mv(fromPath, toPath);
}

/** @inheritDoc @php-wasm/universal!IsomorphicLocalPHP.mv */
adamziel marked this conversation as resolved.
Show resolved Hide resolved
cp(fromPath: string, toPath: string) {
return _private.get(this)!.php.cp(fromPath, toPath);
}

/** @inheritDoc @php-wasm/universal!IsomorphicLocalPHP.rmdir */
rmdir(path: string, options?: RmDirOptions) {
return _private.get(this)!.php.rmdir(path, options);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ export async function installAsset(

// Move asset folder to target path
const assetFolderPath = `${targetPath}/${assetFolderName}`;
await playground.mv(tmpAssetPath, assetFolderPath);
await playground.cp(tmpAssetPath, assetFolderPath, true);
await cleanup();

return {
Expand Down
4 changes: 2 additions & 2 deletions packages/playground/website/cypress/e2e/app.cy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ describe('Query API', () => {
{
step: 'writeFile',
path: '/wordpress/test.php',
data: `<?php
data: `<?php
require("/wordpress/wp-load.php");
echo wp_http_supports(array( "ssl" )) ? "true" : "false";
`,
Expand All @@ -104,7 +104,7 @@ describe('Query API', () => {
cy.wordPressDocument()
.find('[data-slug=gutenberg].active')
.should('exist');
});
}, 120_000);
adamziel marked this conversation as resolved.
Show resolved Hide resolved
});

describe('option `theme`', () => {
Expand Down
Loading