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

Studio: Fix wp-cli command for linux #257

Merged

Conversation

kozer
Copy link
Contributor

@kozer kozer commented Jun 17, 2024

Related to https://github.com/Automattic/dotcom-forge/issues/7327

Proposed Changes

This PR fixes wp-cli execution for linux

Testing Instructions

  1. Ensure that opening a terminal both from bundled and development environment executes commands correctly

Pre-merge Checklist

  • Have you checked for TypeScript, React or other console errors?

Copy link
Contributor

@fluiddot fluiddot left a comment

Choose a reason for hiding this comment

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

I managed to set up a Linux instance and test this PR. I tested different commands both with the app running and without and worked as expected 🎊 . However, I noticed that running the CLI command wp, i.e. without any command results in an error:

Error: '' is not a registered wp command. See 'wp help' for available commands.
Did you mean 'db'?

src/ipc-handlers.ts Outdated Show resolved Hide resolved
src/ipc-handlers.ts Show resolved Hide resolved
@kozer
Copy link
Contributor Author

kozer commented Jun 18, 2024

I managed to set up a Linux instance and test this PR. I tested different commands both with the app running and without and worked as expected 🎊 . However, I noticed that running the CLI command wp, i.e. without any command results in an error:

What is supposed to do in that case @fluiddot ?

@kozer
Copy link
Contributor Author

kozer commented Jun 18, 2024

@fluiddot , I updated the script as it seems that the %q format specifier is not available in all versions of printf

It works now, but please take a look of it in MacOSX as well ( and do a final check on linux )

UPDATE: For some reason, lint fails, but running npm run lint locally, doesn't produce any errors. Can you please also check on mac os as well?

@fluiddot
Copy link
Contributor

What is supposed to do in that case @fluiddot ?

wp should do the same as the command wp help.

@kozer
Copy link
Contributor Author

kozer commented Jun 18, 2024

What is supposed to do in that case @fluiddot ?

wp should do the same as the command wp help.

Yeah, I realized it after the first question, and I fixed it 😄

@fluiddot
Copy link
Contributor

UPDATE: For some reason, lint fails, but running npm run lint locally, doesn't produce any errors. Can you please also check on mac os as well?

Based on the output from CI:

curl: (92) HTTP/2 stream 1 was not closed cleanly: INTERNAL_ERROR (err 2)

Seems it might be a one-off failure, so retrying the jobs should work.

@fluiddot
Copy link
Contributor

@fluiddot , I updated the script as it seems that the %q format specifier is not available in all versions of printf

It works now, but please take a look of it in MacOSX as well ( and do a final check on linux )

Great, thanks! I've reviewed this on both macOS and Linux and worked as expected.

For reference, the processing of the arguments (i.e. command printf '%q ' "$@") was needed for invoking commands with quotes like:

  • wp eval 'echo WP_CONTENT_DIR;'
  • wp eval 'echo rand();'

@fluiddot fluiddot merged commit e4cefe1 into feat/execute-studio-bundled-wp-cli-electron Jun 18, 2024
11 checks passed
@fluiddot fluiddot deleted the fix/linux_wp_cli_terminal branch June 18, 2024 11:21
fluiddot added a commit that referenced this pull request Jun 18, 2024
…213)

* feat: Invoke `wp-cli` from shell sessions originating from Studio

* feat: Add bundled `wp-cli` to macOS search `$PATH`

We cannot modify the search path with environment variables alone, we
must execute an arbitrary script.

* fix: Handle errors from `wp-cli` execution failures

* fix: Prefer bundled `wp-cli` over global `wp-cli`

For shell sessions originating from Studio, the bundled `wp-cli` should
take precedence over a user's existing `wp-cli` installation.

* fix: Prevent launching duplicative Terminal windows

Previously, apparent windows from previous Terminal sessions would
linger and appear when launching a new Terminal.

* feat: Expose bundled `wp-cli` for Windows shell sessions

* fix: Correct Windows `wp-cli` path

The preceding `C:` was duplicated in the CLI path.

* fix: Add `.bat` file ending for Windows

The script must contain the `.bat` file ending in order to run the
script on Windows.

* fix: Update logging of modified new `executeWpCli` method

The internals now return a result rather than logging the result.

* fix: Remove prefix from `wp-cli` error out

Match the approach for `stdout` and avoid displaying unnecessary noise.

* feat: Silence additional console commands

The `executeWpCli` command relies upon `console.warn`, which resulted
in unnecessary noise when invoking the `wp-cli` in the console.

* fix: Remove window reload after CLI command

This logic is likely unnecessary for the current usage with `wp-cli`.

* test: Mock Electron command line methods

* Avoid focusing app when launching second instance with CLI commands

* Prevent unhandled promise when not executing a CLI command on main instance

* Ensure WP-CLI runs the Windows-specific logic

* Add server script path argument to avoid errors in WP-CLI

* Run WP-CLI commands without starting a site

* refactor: Remove unused imports

* fix: Conditionally enforce Windows platform for `wp-cli`

Globally enforcing the Windows platform removed colorization on macOS.

* feat: Conditionally pipe `wp-cli` output through a pager

Mimic core `wp-cli`'s behavior for the `help` command.

* refactor: Remove unnecessary quotation

* Fix working directory when executing wp-cli commands

* Add `shell-quote` package

This library is used to parse the wp-cli arguments.

* Parse CLI arguments using `shell-quote`

* Fix missing quotes when invoking `wp-cli` commands on macOS

* Studio: Fix wp-cli command for linux (#257)

* Studio: Fix wp-cli command for linux

* Update: fix variables per environment

* Update: Fix wp script to handle commands on both MacOS and linux

* Rename CLI env vars with unique names

* Fix paging for command `wp help`

* Log `wp-cli` command errors using `console.error`

---------

Co-authored-by: Carlos Garcia <fluiddot@gmail.com>
Co-authored-by: Antony Agrios <antonyagrios@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants