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: Download the latest wp-cli and bundle it #178

Merged
merged 3 commits into from
May 29, 2024
Merged

Conversation

kozer
Copy link
Contributor

@kozer kozer commented May 28, 2024

Fixes https://github.com/Automattic/dotcom-forge/issues/7320

Proposed Changes

This PR do the following:

  1. Adds wp-cli in the studio bundle.
  2. Extract and update it when the user starts the app.

Testing Instructions

  1. Run nvm use && npm i
  2. Notice that the WP-CLI has been downloaded in wp-files/wp-cli folder.
  3. Run npm run package
  4. Ensure that wp-cli.phar exists in ./out/<architecture>/resources folder under wp-files folder.
  5. Start studio app.
  6. Ensure that the wp-cli.phar is updated and is now located under ~/.config/Studio/server-files directory.

Pre-merge Checklist

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

@kozer kozer requested a review from a team May 28, 2024 12:09
@kozer kozer self-assigned this May 28, 2024
Copy link
Member

@sejas sejas left a comment

Choose a reason for hiding this comment

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

@kozer , thank you for creating this PR.
I tested it and the bundled .phar is present in wp-files when running npm install and also in the App resources.

Screenshot 2024-05-28 at 21 11 20 Screenshot 2024-05-28 at 21 11 39

I left a comment asking if is possible to use that file directly, instead of copying it to the "home" directory.

When starting the app I also confirm the app downloads and replaces the .phar file in the home directory. But I think we don't need that behaviour. My concern is that it could make the app starting slower.

File at "~/Library/Application Support/Studio/server-files/wp-cli.phar"

Screenshot 2024-05-28 at 20 54 32

scripts/download-wp-server-files.ts Show resolved Hide resolved
src/setup-wp-server-files.ts Outdated Show resolved Hide resolved
src/setup-wp-server-files.ts Outdated Show resolved Hide resolved
src/setup-wp-server-files.ts Outdated Show resolved Hide resolved
@kozer
Copy link
Contributor Author

kozer commented May 29, 2024

Thank you @sejas for your comments! I updated the PR! Can you test it again? Thanks!

@kozer kozer requested review from sejas and wojtekn May 29, 2024 09:51
@sejas sejas requested a review from a team May 29, 2024 10:06
Copy link
Member

@sejas sejas left a comment

Choose a reason for hiding this comment

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

Looking great. Thanks for the changes @kozer
I tried it by running npm run make
I confirm the bundled version exists and I even executed inside Studio adding some code.

getWpCliPath(): /Users/macbookpro/Downloads/Studio.app/Contents/Resources/wp-files/wp-cli/wp-cli.phar
WP-CLI 2.10.0

@kozer kozer merged commit 547c532 into trunk May 29, 2024
12 checks passed
@kozer kozer deleted the feat/bundle_wp_cli branch May 29, 2024 10:31
@wojtekn
Copy link
Contributor

wojtekn commented May 30, 2024

@sejas @kozer what is the reason and benefit of handling WP CLI in a different way than WP core and SQLite plugin?

@kozer
Copy link
Contributor Author

kozer commented May 30, 2024

@sejas @kozer what is the reason and benefit of handling WP CLI in a different way than WP core and SQLite plugin?

Hey @wojtekn ! The benefit I see here, is that, as we don't update it currently, there is no reason to add overhead when we start the app, to move thing around.
I believe thought, that when we add update into the mix, this is gonna change, as it most probably won't be able to update it as is, and we ll need to copy it into the home folder first, as implemented in my first commit under this PR.

@wojtekn
Copy link
Contributor

wojtekn commented May 30, 2024

@kozer, so why would we do this in the first place? Wouldn't it be beneficial to have it consistent for all three libraries, what I pointed out in #178 (comment) comment?

@kozer
Copy link
Contributor Author

kozer commented May 30, 2024

@kozer, so why would we do this in the first place? Wouldn't it be beneficial to have it consistent for all three libraries, what I pointed out in #178 (comment) comment?

The benefit is only temporary, but it is valid, as is. Not a big deal though, as it will probably change soon.
If you think this is not ok, I can quickly restore that back.
What do you think?

@sejas
Copy link
Member

sejas commented May 30, 2024

we can restore the copyBundledWPCLI call for consistency , but let's address the update logic in a different issue https://github.com/Automattic/dotcom-forge/issues/7326.

In this commit we were moving the wp-cli.phar and inmediatly downloading it from the network every time the app started, and we need to add a more complex logic to compare wp-cli versions.

The biggest argument to keep wp-cli.phar inside the app is that its an "executable" globally available and not something attached to each site.

@kozer
Copy link
Contributor Author

kozer commented May 31, 2024

we can restore the copyBundledWPCLI call for consistency , but let's address the update logic in a different issue https://github.com/Automattic/dotcom-forge/issues/7326.

In c68ca83#diff-c1a4ebb77029d831401b5e40b14a997a07bed19b01940a7b0210dd12c33178acL63-L75 we were moving the wp-cli.phar and inmediatly downloading it from the network every time the app started, and we need to add a more complex logic to compare wp-cli versions.

The biggest argument to keep wp-cli.phar inside the app is that its an "executable" globally available and not something attached to each site.

I started working on that. I'll handle everything in this ticket.

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.

None yet

3 participants