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

Feature/arm #92

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from
Open

Feature/arm #92

wants to merge 3 commits into from

Conversation

dustinrue
Copy link
Contributor

Description of the Change

This PR modifies the Dockerfile and build routines (actions) to:

  • Build the image off of the updated upstream image 10up/wp-php-fpm-dev:7.4-ubuntu
  • Provide a multi-stage build to reduce overall image size (using 10up/base-php:7.4-ubuntu)
  • Updates the Github action to build for both amd64 and arm64 (adding native, full speed support for Apple's M series of processor including M1)

Alternate Designs

Benefits

Full native speed for Apple M1

Possible Drawbacks

Verification Process

I'm unable to fully test this

Checklist:

  • I have read the CONTRIBUTING document.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my change.
  • All new and existing tests passed.

Applicable Issues

Changelog Entry

  • Update base image and create multi-arch image compatible with arm64 and Apple M series processors

@tylercherpak tylercherpak self-requested a review November 17, 2021 15:48
@dustinrue
Copy link
Contributor Author

To test this you would need to update the images value in wp-local-docker to ensure this image, where ever it lives, it used. I have a test version of this at https://hub.docker.com/repository/docker/dustinrue/wp-snapshots


COPY --from=builder --chown=wpsnapshots=wpsnapshots /opt/wpsnapshots /opt/wpsnapshots
RUN \
apt-get install mariadb-client -y && apt-get clean all && \

Choose a reason for hiding this comment

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

I could not build until I added apt-get update here. The mariadb-client package was not found.

Suggested change
apt-get install mariadb-client -y && apt-get clean all && \
apt-get update && apt-get install mariadb-client -y && apt-get clean all && \

@NicktheGeek
Copy link

I was able to test this and it solves a couple of important problems.

  1. PHP 7.3 fails during export when encountering code in the site that is built for php 7.4+
  2. M1 chips throw a non failing error but are subject to performance issues

This is the process I used to test.

  1. Delete my local snapshot docker image
  2. Clone the repo and checkout the branch
  3. Modified the docker file with apt-get update && as noted in this comment Feature/arm #92 (comment)
  4. Built and tagged as 10up/wpsnaphots:latest so I could use that image with this command docker build -t 10up/wpsnapshots:latest . --build-arg WPSNAPSHOTS_ARCHIVE="https://github.com/10up/wpsnapshots/archive/refs/tags/2.2.1.tar.gz"
  5. Ran snapshots on a site that was failing due to php 7.4 code

This worked for me and I think this should be merged and deployed once the apt-get update part is added so it will build successfully. Until then, I'll be writing instructions for my team to update their local image to a working version.

As a side note, might be a good idea to have multiple versions tagged for php version so that projects using php 8 and even php 8.1 can work correctly. This would require an update for 10up local docker so the php version of the project affects the version used to run snapshots.

darylldoyle added a commit that referenced this pull request Jun 13, 2023
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.

2 participants