-
Notifications
You must be signed in to change notification settings - Fork 9
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
ci: 👷 changed to building imagick from source #2428
ci: 👷 changed to building imagick from source #2428
Conversation
.kube/app/Dockerfile
Outdated
@@ -1,6 +1,7 @@ | |||
FROM php:8.4-fpm | |||
|
|||
ENV PHP_VERSION 8.4 | |||
ENV IMAGICK_VERSION 3.7.0 # This should be bumped to the latest version and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems like part of the comment is missing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the best way to test these changes before merging?
|
||
RUN pecl install imagick | ||
# START BUILDING IMAGICK FROM SOURCE - this can be removed once pecl fixes issues installing with php8.4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please leave a link to #2425 in the comments. It'll just make it easier to understand when scanning the code later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added
.kube/app/Dockerfile
Outdated
# - this can be removed once pecl fixes issues installing with php8.4 | ||
RUN apt-get remove -y \ | ||
$PHPIZE_DEPS \ | ||
libtool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to do the remove in the same block as the install? I think it's more likely this will get missed when reverting later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It needs to be there or the following pecl build commands fail. I thought those commands ensures build dependencies were installed if necessary when pecl runs, but it failed when I removed them prior to running those.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Migrated to TODO comments which should be recognized by IDE's that implement finding the tags for pending items that are pending changes.
.kube/app/Dockerfile
Outdated
@@ -1,6 +1,8 @@ | |||
FROM php:8.4-fpm | |||
|
|||
ENV PHP_VERSION 8.4 | |||
# TODO - IMAGICK_VERSION can be removed once pecl fixes issues installing with php8.4 | |||
ENV IMAGICK_VERSION 3.7.0 # This should be bumped to the latest version, especially when PHP version is bumped and can be removed when https://github.com/accessibility-exchange/platform/issues/2425 is resolved |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this link is a bit odd because this PR will close that issue. Maybe reword a bit here so that it indicates that 2425 is what this work around is for an can be removed when the upstream issue in imagick is resolved (and link to an imagick issue).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed all the TODO comments to reference issue Imagick/imagick#698.
.kube/app/Dockerfile
Outdated
|
||
RUN mkdir -p /tmp/imagick | ||
WORKDIR /tmp/imagick | ||
RUN curl -L -o /tmp/imagick.tar.gz https://github.com/Imagick/imagick/archive/tags/${IMAGICK_VERSION}.tar.gz |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I tried to build the container I got the error
failed to solve: process "/bin/sh -c curl -L -o /tmp/imagick.tar.gz https://github.com/Imagick/imagick/archive/tags/${IMAGICK_VERSION}.tar.gz" did not complete successfully: exit code: 6
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When abstracting version to an environment variable it broke the curl command reverted to static url.
@marvinroman I'm having trouble testing this with the local docker compose. Every time I get to a spot that needs to start mysql it fails. It says it can't properly start it or that it's unhealthy. Have you run into an issue like this? |
@marvinroman I've tried a few more times after updating my docker tools and it is still failing to start mysql. I asked @greatislander to also try this PR but he was unable to run the local docker compose based environment due to issues running it on an Apple Silicon based Mac. |
It's not just an issue with Apple and I was able to resolve the issue by eliminating the custom MySQL configuration but this configuration is the same as what gets deployed on the Kube. I ran out of time last week to finish the full update and to find out which configuration variable it causing the issues. I will submit changes this week to resolve. |
@jobara The reported errors in deployment have been fixed in branch infrastructure-local-environment-update-nix you will want to merge both these branches locally to be able to fully build and deploy. To complete the updates I would do the following after downloading the updates.
|
@marvinroman I got things built using nix, but even when combining the PRs I still got a failure with mysql ( |
Can you give me the contents of You should no longer need Did you delete the |
@marvinroman The issue I'm mentioning here is when running without nix ( I did get something to build using nix and have merged #2429 ) and using just the instructions from "Local development setup using docker compose:". I still get the issues I mentioned above with the ID & Group needing to happen earlier and mysql failing to start. |
@marvinroman here are the logs from
|
@marvinroman just a heads up that the laravel-vite-plugin was recently updated with this change ( laravel/vite-plugin#318 ). I don't know if it will affect any of the local docker or nix setups, but just letting you know in case. |
It shouldn't. |
@jobara I would rebase or update from dev both those issues should be addressed by the new nix environment. |
@marvinroman Yes I've been testing by first creating a test branch off of dev and merging this PR's branch into it. The problem here though is that there is still documentation, and I suppose artifacts in the repo, for running "Local development setup using docker compose". If this method for local development is no longer supported all those parts should be removed. |
@jobara That is true. Initially I wanted to keep both to not make Nix a requirement. But I think some of it broke with the new environment and it's better to not try and maintain both options. That section is now removed. |
…of-imagick-library
Updated Dockerfile that builds the main application image to build the imagick library from source instead of using
pecl
since their version is not yet fully ready for php v8.4.