-
Notifications
You must be signed in to change notification settings - Fork 41
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
wp-now: Update Playground and PHP WASM dependencies #350
wp-now: Update Playground and PHP WASM dependencies #350
Conversation
…andler, file system mount handler - Based on similar logic in `@wp-playground/wordpress` and `cli`
Currently there are 3 failing tests, as seen in the CI logs. They're about running PHP as CLI, and about WP-CLI.
There are also WASM errors associated with the PHP CLI tests, both:
That could be related to a similar error described in: I'll copy the WASM error stack traces below for reference. They're slightly different, so might have different causes. stderr | src/tests/wp-now.spec.ts > Test starting different modes > validate php comand arguments passed through yargs > php should execute a file
stderr | src/tests/wp-now.spec.ts > Test starting different modes > validate php comand arguments passed through yargs > php should execute a file and change php version
I checked the called I saw that a similar update of Playground and PHP-WASM dependencies happened in: The project includes a vendored and patched One difference I noticed is setting up TLS root certificates. I'll try that and some other relevant changes, and see if it helps with passing the remaining tests. |
@sejas poke for the reviews! :-) |
As for the |
- Use TLS certificates - Workaround for stdin/out/err - Not implemented yet: SQLite command See https://github.com/Automattic/studio/blob/trunk/vendor/wp-now/src/execute-wp-cli.ts
OK, down to 2 failing tests, both about PHP CLI and WASM errors.
I need help with solving the WASM part - as far as I can see, all the C functions are already listed in For the WP-CLI test that was failing before, I ported the changes in It includes a somewhat gnarly workaround to make WP-CLI work. Some of this might be suitable to move upstream as shared logic in One part I had to comment out, about SQLite command. Maybe this could be left for another PR to more fully develop the WP-CLI integration, which is (I think) not yet part of the public interface of |
Here are the functions listed in the WASM error stacks (below). I combined and sorted them alphabetically, then checked each against the
The missing one is I can create a PR for this - but it might be better to try locally first, and see if it actually solves the issue. I guess in my local There's an ongoing PR that updates the same This could be the quickest solution to ask if they can add
|
Next steps to complete this PR:
Even after adding
I added these to |
By the way, I'm finding it helpful to use Bun to run functions during development, without having to build the project or run the test suite every time. In a parent directory, a quick script to try things: import path from 'node:path'
import getWpNowConfig, { WPNowMode } from './playground-tools/packages/wp-now/src/config'
import { executePHP } from './playground-tools/packages/wp-now/src/execute-php'
import { executeWPCli } from './playground-tools/packages/wp-now/src/execute-wp-cli'
const options = await getWpNowConfig({
path: path.join(import.meta.dir, 'test'),
port: 3000,
reset: true,
})
console.log(
await executeWPCli(['cli', 'version'])
)
await executePHP(['php', '--version'], options)
await executePHP(['php', `--help`], options)
await executePHP(['php', `-r`, `echo 'hi';`], {
...options,
mode: WPNowMode.INDEX,
})
await executePHP(
['php', `-r`, `echo json_encode( scandir('${options.documentRoot}') );`],
options,
) This last CLI command throws a different error than in the tests. Maybe the stack trace contains a hint about how to solve it.
Here's the complete list of
In my fork of
For some reason that doesn't solve it. |
All tests pass when PHP CLI is run in "index" mode. Other modes (playground, plugin, theme, wordpress) fail with WASM error. That makes me think, maybe the issue is not with missing functions but in the way the PHP instance is set up before calling the CLI. |
- Based on changes in Automattic/studio: https://github.com/Automattic/studio/blob/trunk/vendor/wp-now/src/execute-wp-cli.ts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@eliot-akira, excellent work! Huge kudos for your perseverance and dedication in troubleshooting all the errors. I truly admire your effort.
After implementing the changes to the WP-CLI test, I can confirm that all tests pass when running the command npx nx test wp-now
.
Changes
packages/wp-now/src/tests/wp-now.spec.ts
...
/**
* Test wp-cli command.
*/
describe('wp-cli command', () => {
beforeAll(async () => {
await downloadWithTimer('wp-cli', downloadWPCLI);
});
afterAll(() => {
fs.removeSync(getWpCliTmpPath());
});
/**
* Test wp-cli displays the version.
* We don't need the WordPress context for this test.
*/
test('wp-cli displays the version', async () => {
const { stdout } = await executeWPCli([ '--version' ], {projectPath: '.'});
expect(stdout).toMatch(/WP-CLI (\d\.?)+/i);
});
});
Tests
❯ npx nx test wp-now
> nx run wp-now:test
RUN v0.31.1 /Users/macbookpro/Documents/projects-m3.nosync/playground-tools/packages/wp-now
✓ src/tests/add-trailing-slash.spec.ts (3 tests) 4ms
✓ src/wp-playground-wordpress/tests/is-valid-wordpress-version.test.ts (1 test) 2ms
✓ src/wp-playground-wordpress/tests/get-plugin-file.spec.ts (3 tests) 2ms
✓ src/tests/github-codespaces.spec.ts (2 tests) 2ms
✓ src/tests/execute-php.spec.ts (4 tests) 222ms
stdout | unknown test
Downloading wordpress...
Downloading sqlite...
stdout | unknown test
sqlite downloaded.
sqlite: 891.667ms
stdout | unknown test
wordpress downloaded.
wordpress: 3.856s
✓ src/tests/wp-now.spec.ts (46 tests | 1 skipped) 54354ms
Test Files 6 passed (6)
Tests 58 passed | 1 skipped (59)
Start at 10:55:11
Duration 54.86s (transform 116ms, setup 0ms, collect 586ms, tests 54.59s, environment 834ms, prepare 185ms)
——————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————
> NX Successfully ran target test for project wp-now (55s)
I also tested wp-now common functionality and I could run a WordPress site and create a new post by running: npx nx build wp-now && node dist/packages/wp-now/cli.js start
I left a few comments, and I think we are close to merge it.
I couldn't reproduce the WASM errors you mentioned. Can you share some testing steps to reproduce them?
… action, php.ini, improve rotate runtime logic - See https://github.com/Automattic/studio/blob/trunk/vendor/wp-now/src/wp-now.ts
OK, I applied the suggested changes. Thank you for the review. I ported some useful changes from the Studio fork of wp-now, including support for rewrite rules and file not found action; and improvements to php.ini setup and rotate runtime instances with shared init steps. Getting closer.. All tests passing, except:
I temporarily "solved" it by forcing PHP CLI to run in I'll remove that patch so the CI shows the errors again. I'm not sure how to solve this one, as far as I could tell, adding functions to PHP-WASM and recompiling didn't eliminate the error. |
Thanks for the quick updates! 🙏 |
Poke @sejas :) |
I updated Playground dependencies to the newest version The WASM error stack trace looks the same as before. I'll try again to solve it, apparently it happens with any
Strangely, the CI step On my local machine it's passing. |
@eliot-akira , thanks for upgrading to I tested it in my laptop. I'm getting the same errors you mentioned, and one more error about compressing ( Currently these are the errors I get when running the wp-now tests Full error trace
What is weird is that in CI it seems to (fail)(https://github.com/WordPress/playground-tools/actions/runs/11314731564/job/31465056405?pr=350) on
While in my local machine, those typecheck pass: |
I've updated I also fixed the tests that were checking if express was compressing files before sending them in the request. (a73938c) Let's see if the |
It seems the CI rebases from trunk, that's why we didn't get the error in our local machines. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested it in Playground, theme and plugin modes.
It works as expected. Great work @eliot-akira 🥳
❯ npx nx build wp-now && node dist/packages/wp-now/cli.js start
> nx run wp-now:"build:bundle"
> nx run wp-now:"build:package-json"
> nx run wp-now:build [local cache]
———————————————————————————————————————————————————————————————————————————————————————————————————————————————————————
> NX Successfully ran target build for project wp-now and 2 tasks it depends on (472ms)
Nx read the output from the cache instead of running the command for 1 out of 3 tasks.
Starting the server......
directory: /Users/macbookpro/Documents/projects-m3.nosync/playground-tools
mode: playground
php: 8.0
wp: 6.6.2 (resolved from alias: latest)
WordPress 6.6.2 folder already exists. Skipping download.
SQLite folder already exists. Skipping download.
Server running at http://localhost:8881
Whew, that was a tough one. 😄 Thank you for seeing it through! |
Thank YOU @eliot-akira for working hard on it 🙏 . I couldn't publish the release yet, I got some errors in the build process. I created a PR #359 but I think I also caught a bug in |
<!-- Thanks for contributing to WordPress Playground Tools! --> ## What? <!-- In a few words, what is the PR actually doing? Include screenshots or screencasts if applicable --> It updates the correct path of `jspi/PHP_*` to be included in the VS Code extension after upgrading the Playground dependencies on #350 ## Why? <!-- Why is this PR necessary? What problem is it solving? Reference any existing previous issue(s) or PR(s), but please add a short summary here, too --> The paths to wasm files have changed in the new version. ## How? <!-- How is your PR addressing the issue at hand? What are the implementation details? --> Updating the project bundle step. ## Testing Instructions <!-- Please include step by step instructions on how to test this PR. --> <!-- 1. Check out the branch. --> <!-- 2. Run a command. --> <!-- 3. etc. --> * Run `npx nx run vscode-extension:build:bundle` * Observe the command finishes successfully : ` > NX Successfully ran target build:bundle for project vscode-extension (501ms)`
What?
This pull request is a continuation of #345 to update Playground dependencies from
0.6.16
to1.0.2
and adapt existing code to work with breaking changes.Resolves #319.
Why?
To keep up with upstream changes and benefit from new features and bug fixes.
How?
Update to new interface for PHP runtime instances, requests, and file system mount.
Based on similar logic in
@wp-playground/wordpress
(bootWordpress
) andcli
.PHP
class from@php-wasm/universal
instead ofNodePHP
from@php-wasm/node
, which no longer exists.PHPRequestHandler
instead ofphp.request()
createNodeFsMountHandler
to mount the file system with Node-specific adaptor forphp.mount()
Further changes were necessary to pass all existing tests.
executeWpCli
based on fork ofwp-now
in Automattic/studioUpdate@php-wasm/compile
to add any missing C functions to the list inASYNCIFY_IMPORTS
in the Dockerfile here.Testing Instructions
npx nx test wp-now