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

wp-now: Add executeWPCli() function to download and execute WP-CLI #395

Merged
merged 24 commits into from
May 30, 2023

Conversation

sejas
Copy link
Collaborator

@sejas sejas commented May 19, 2023

What?

  • Add the executeWPCli function to download and execute `wp-cli.
  • In the future, we may include the command wp-now wp. Currently, we drop it out until we improve the pthreads execution. Currently a PR in progress: PHP: Explore building with pthreads #346
  • Surface emscriptenOptions to catch print and print error for wp-cli execution.

Why?

How?

It downloads the wp-cli.phar file if the file doesn't exist, then uses php.cli() to execute it.
There are some limitations in the wp-cli features. Some of them may not work.

Testing Instructions

  • Check out this branch.
  • Copy your path to your theme or plugin
  • After installing and building the project, run:
  • Run the tests npx nx test wp-now
  • Observe the tests pass.

@sejas sejas self-assigned this May 19, 2023
@sejas sejas marked this pull request as ready for review May 23, 2023 14:37
@danielbachhuber danielbachhuber changed the title wp-now: add wp-cli command wp-now: Add executeWPCli() function to download and execute WP-CLI May 24, 2023
@adamziel
Copy link
Collaborator

@sejas is that callback just for unit tests? If yes, you could mock process.stdout instead

@sejas
Copy link
Collaborator Author

sejas commented May 24, 2023

@sejas is that callback just for unit tests? If yes, you could mock process.stdout instead

@adamziel , yes this callback is just for the unit test.
I tried it to mock the process.stdout but I couldn't make it work.

So I decided to go with the option of onStdout.

99174cc

Some things I tried:

process.stdout.on('data', collectOutput);
process.stdout.off('data', collectOutput);
 let stdoutWriteMock;

  beforeEach(() => {
    stdoutWriteMock = vi.spyOn(process.stdout, 'write').mockImplementation((buffer: Buffer | string, cb?: ((err?: Error) => void) | undefined) => {
      if (cb) cb();
      return true;
    });
  });

  afterEach(() => {
    stdoutWriteMock.mockRestore();
  });

Maybe a related bug: jestjs/jest#9984

@adamziel
Copy link
Collaborator

adamziel commented May 24, 2023

@sejas oh, sorry, that wasn't process.stdout. Here's what Emscripten does by default:

var out = Module["print"] || console.log.bind(console);
var err = Module["printErr"] || console.warn.bind(console);

Mocking console.log should work better

@sejas
Copy link
Collaborator Author

sejas commented May 24, 2023

@adamziel , Yay!, it worked. I mocked the console.log. Thanks for the direction.

@sejas
Copy link
Collaborator Author

sejas commented May 24, 2023

@danielbachhuber , @adamziel , I think this PR is ready to merge.

if (process.env.NODE_ENV !== 'test') {
return path.join(getWpNowPath(), 'wp-cli.phar');
}
return path.resolve('./wp-cli.phar');
Copy link
Member

Choose a reason for hiding this comment

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

Should this be a temp directory?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I found that using a temporal directory on my laptop, the wp-cli.phar cannot be executed.
I tried setting the right permissions fs.chmod() to the file and folder and it didn't work.

Copy link
Member

Choose a reason for hiding this comment

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

@sejas Ok. I think we should use a tmp directory in any case. I added with 5e8e97c

@adamziel Any idea why the test might fail?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@danielbachhuber , with the temp folder, it seems to pass ok in CI. But it is still failing on my local computer.
Maybe it's something just in my environment.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it getting mounted correctly @sejas? What do you see when you list files?

Copy link
Member

Choose a reason for hiding this comment

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

@sejas It failed locally for me too.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Is it getting mounted correctly @sejas? What do you see when you list files?

Yes, the file is accessible in VFS and the file size is correct. It must be something related to MacOs and the tmp path.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@sejas would you start a new issue for this? I'd like to make sure this works on MacOs

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There was a conflict mounting /var and using the hostFilesystem.
A second PR was needed:
WordPress/playground-tools@bf31e3e

@danielbachhuber danielbachhuber requested a review from adamziel May 30, 2023 18:14
...args,
]);
} catch (resultOrError) {
const success =
Copy link
Collaborator

Choose a reason for hiding this comment

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

FYI I merged this change: https://github.com/WordPress/wordpress-playground/pull/470/files#diff-3652c54df4b8d0d8e367481121e32f08cc409cfc4236fdb3172d69dcbf6ca8b0

It shouldn't matter here at all and this check still passes, but I wanted to flag it here just in case.

@@ -2,7 +2,7 @@
"extends": "./tsconfig.json",
"compilerOptions": {
"outDir": "../../dist/out-tsc",
"types": ["jest", "node"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

nice

@adamziel adamziel merged commit 6170984 into WordPress:trunk May 30, 2023
@adamziel
Copy link
Collaborator

wp-cli support 💯

@sejas sejas deleted the add/wp-now-command-wp-cli branch May 31, 2023 09:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants