Skip to content
This repository was archived by the owner on Jun 1, 2023. It is now read-only.

Fix for "X zip is required for packaging a theme" on Windows#2667

Merged
karreiro merged 1 commit intomainfrom
fix-2513
Oct 26, 2022
Merged

Fix for "X zip is required for packaging a theme" on Windows#2667
karreiro merged 1 commit intomainfrom
fix-2513

Conversation

@mgmanzella
Copy link
Copy Markdown
Contributor

@mgmanzella mgmanzella commented Oct 19, 2022

WHY are these changes introduced?

Fixes #2513

WHAT is this pull request doing?

Modifies the package command to use a 7zip to zip files when theme package is run on Windows

How to test your changes?

Machine Setup

The test steps should be run on all platforms.

Mac
  • Clone shopify-cli repo
  • Add the following to ~/.zshrc and replace with your machine username:
alias shopify-dev="/Users/<USERNAME>/src/github.com/Shopify/shopify-cli/bin/shopify"
Windows
  • Clone shopify-cli repo and checkout branch with changes
  • Change shopify.bat name in the bin folder of the repo to shopify-dev.bat
  • Add the bin folder to the PATH
  • Restart machine
  • Confirm setup works by running shopify-dev version
Linux

TODO

Test Steps

  1. If you don't have a theme available, shopify-dev theme init
  2. shopify-dev theme package <path/to/theme>
  3. Confirm zip file is generated in theme directory with theme name and version as the name
  4. Unzip the zip file and confirm only directories in THEME_DIRECTORIES are present
  5. Confirm nested folders were also included
  6. cd <theme-name>
  7. shopify-dev theme package
  8. Repeat steps 3-5

Post-release steps

  • Open issue in shopify-dev to update theme package docs to indicate we only support 7zip for theme package on windows

Update checklist

  • I've added a CHANGELOG entry for this PR (if the change is public-facing)
  • I've considered possible cross-platform impacts (Mac, Linux, Windows).
  • I've left the version number as is (we'll handle incrementing this when releasing).
  • I've included any post-release steps in the section above (if needed).

@mgmanzella mgmanzella changed the title Fix theme package to work on windows Fix for "X zip is required for packaging a theme" on Windows Oct 19, 2022
@mgmanzella mgmanzella marked this pull request as ready for review October 19, 2022 22:18
@mgmanzella mgmanzella requested review from a team October 19, 2022 22:18
@karreiro karreiro self-requested a review October 20, 2022 11:36
Copy link
Copy Markdown
Contributor

@karreiro karreiro left a comment

Choose a reason for hiding this comment

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

Great stuff, @mgmanzella! 🚀 LGTM and works on Windows as well :)

What do you think about handling 7z as a fallback of the zip command?

Some Linux distributions might not have the zip command, but they may have 7z, so we'd cover this case.

Thank you for this PR!

@mgmanzella mgmanzella force-pushed the fix-2513 branch 2 times, most recently from 288ed4a to 1e61b08 Compare October 21, 2022 22:52
Copy link
Copy Markdown
Contributor Author

@mgmanzella mgmanzella left a comment

Choose a reason for hiding this comment

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

What do you think about handling 7z as a fallback of the zip command?

great idea!! @karreiro i made changes to do this and left some comments for you to look at 🙏

Comment thread lib/project_types/theme/commands/package.rb
Comment thread lib/project_types/theme/commands/package.rb
Comment thread lib/project_types/theme/messages/messages.rb
Copy link
Copy Markdown
Contributor

@karreiro karreiro left a comment

Choose a reason for hiding this comment

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

Thank you for the updates, @mgmanzella!

I've added a minor comment about the which method :)

Comment thread lib/project_types/theme/commands/package.rb
Comment thread lib/project_types/theme/commands/package.rb
@mgmanzella mgmanzella force-pushed the fix-2513 branch 3 times, most recently from d303c4d to 967eb55 Compare October 24, 2022 20:27
@mgmanzella mgmanzella requested a review from karreiro October 24, 2022 20:28
Comment thread lib/project_types/theme/commands/package.rb
Copy link
Copy Markdown
Contributor

@karreiro karreiro left a comment

Choose a reason for hiding this comment

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

Thanks a lot, @mgmanzella! LGMT and works greatly too!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: X zip is required for packaging a theme. Please install zip using the appropriate package manager for your system.

2 participants