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 a cross-device mv error by using a tmp directory inside document root #886

Merged
merged 2 commits into from
Dec 20, 2023

Conversation

adamziel
Copy link
Collaborator

@adamziel adamziel commented Dec 19, 2023

What is this PR doing?

Solves the error discovered by @sejas caused by calling php.mv(fromPath, toPath) when fromPath is a mounted local directory and toPath is a MEMFS directory. Emscripten doesn't support such a scenario:

Proceeding without the Notification plugin. Could not install it in wp-admin. The original error was: Error: Could not move "/tmp/assets/Notification/notification": Cross-device link.
Error: Could not move "/tmp/assets/Notification/notification": Cross-device link.
    at descriptor.value (/playground-tools/node_modules/@php-wasm/node/index.cjs:67481:17)

The solution proposed in this PR replaces a /tmp directory with a randomly-named temporary directory inside wp-content. /tmp doesn't necessarily point to a system temp directory and needs to be revisited anyway. Whether we should use a temporary directory inside wp-content is another matter, but that part may be revisited once the recursive cp feature is added.

Testing instructions

@adamziel adamziel self-assigned this Dec 19, 2023
@adamziel
Copy link
Collaborator Author

I just confirmed this fixes Blueprints in wp-now cc @sejas:

CleanShot 2023-12-20 at 00 51 50@2x

@adamziel adamziel merged commit f5082bf into trunk Dec 20, 2023
5 checks passed
@adamziel adamziel deleted the fix-cross-device-mv-error branch December 20, 2023 09:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant