use libgd instead of ImageMagick #3065

akostadinov · 2022-09-15T18:18:29Z

Note: DEPLOY ONLY AFTER COORDINATION WITH PRODUCT TEAM!

In RHEL8+ ImageMagick is no longer provided due to being hard
to maintain and prone to security issues.

That's why we implement an image processor based on the libgd backed
local-fastimage_resize gem for logo processing instead of the
Paperclip's default ImageMagick based processor Paperclip::Thumbnail.

We remove support for uploading GIF profile logos in the process.
Logo is used when generating invoices. But prawn only supports JPEGs
and PNGs.

With ImageMagick we could specify target format for the :invoice
style to be png regardless of source image format.
local-fastimage_resize only resizes between same formats though.

Provided very low adoption of GIF logos, it is not worth implementing
such support for local-fastimage_resize although it shouldn't be too
hard if need arise.

Note: existing attachments don't need to be processed because the
:invoice style will already be PNG. Make sure NOT to re-process all
attachments though. I don't see a reason to re-process them.
Note: existing png and gif attachments don't need to be processed because the
:invoice style will already be PNG. JPEGs will need though as style for them changes.

fixes THREESCALE-8579

test/integration/provider/admin/account/logos_controller_test.rb

INSTALL.md

test/unit/tasks/fixes_test.rb

mayorova · 2022-09-23T13:26:04Z

There is also a conflict in INSTALL.md

app/models/profile.rb

mayorova · 2022-09-23T14:44:37Z

Maybe we could do something to improve UX, in this branch uploading a .gif file shows

Maybe we could add some message saying which formats are supported (especially as we are dropping gifs).

Or probably we can leave it for another PR, or even leave it as it is - as at the moment for all other non-supported types of logos we are showing the same error.

INSTALL.md

mayorova · 2022-09-24T21:14:16Z

lib/tasks/fixes.rake

+  task :regenerate_jpeg_invoice_logo => :environment do
+    Profile.where(logo_content_type: "image/jpeg").find_each do |profile|
+      begin
+        profile.logo.reprocess!(:invoice)


I am wondering if we are affected by this bug: thoughtbot/paperclip#2637

I have tested this successfully though, so not sure if this may cause any issues. But this is to be aware of for SaaS migration.

I think we should be fine. Since we are not running an external process like it was done by ImageMagic, I think in case of a trouble, the process will just fail. Also if target style only is truncated, that shouldn't be an issue.

But I will add as a precaution to backup the S3 bucket beforehand. Good catch!

I managed to test this by editing gems/paperclip-6.1.0/lib/paperclip/storage/s3.rb and making sure an empty local file is being generated when copying the file locally.

The bug is present - paperclip overwrites the target image with an empty file.

But we have nothing to worry because:

it only overwrites the styles being reprocessed, this means that original logo will not be affected

because the invoice style will be missing anyway, overwriting it with a zero byte file will not matter (no data will be lost)

What we have to do would be to check for any broken invoice logos after running the task.

mayorova · 2022-09-24T21:46:53Z

Great job @akostadinov ! 👏
LGTM 👍

nidhi-soni1104 · 2022-09-26T07:27:17Z

lib/tasks/fixes.rake


+  desc "Regenerate JPEG logos :invoice style"
+  task :regenerate_jpeg_invoice_logo => :environment do
+    Profile.where(logo_content_type: "image/jpeg").find_each do |profile|


What about png extension?

josemigallas · 2022-11-02T07:55:00Z

@thalesmiguel Didn't you have any issues installing local-fastimage_resize on your mac? Merging this PR would essentially break my local env.

thalesmiguel · 2023-02-28T10:05:38Z

I managed to install the local-fastimage_resize gem with gem install local-fastimage_resize -- --with-opt-dir=$(brew --prefix gd). After that, bundle [install] went smoothly and I was able to run the application.
@akostadinov I can give it another try after the merge conflicts are solved if you want.

josemigallas · 2023-03-13T15:25:11Z

I managed to install the local-fastimage_resize gem with gem install local-fastimage_resize -- --with-opt-dir=$(brew --prefix gd). After that, bundle [install] went smoothly and I was able to run the application.

It works! 👍🏻

josemigallas · 2023-04-24T15:17:56Z

osx-manual-setup.md


 ```
-brew install chromedriver imagemagick@6 gs pkg-config openssl geckodriver sphinx [email protected] postgresql@14
+brew install chromedriver imagemagick@6 gs pkg-config openssl geckodriver sphinxgd [email protected] postgresql@14


Wait, shouldn't we remove imagemagick and add gd?

Suggested change

brew install chromedriver imagemagick@6 gs pkg-config openssl geckodriver sphinxgd [email protected] postgresql@14

brew install chromedriver libgd gs pkg-config openssl geckodriver sphinxgd [email protected] postgresql@14

in the next one #3165

But wait isn't package called gd?
https://formulae.brew.sh/formula/gd

josemigallas · 2023-04-24T15:18:10Z

osx-manual-setup.md

 bundle config --local build.thin --with-cflags=-Wno-error="implicit-function-declaration"
 bundle config --local build.github-markdown --with-cflags=-Wno-error="implicit-function-declaration"
 bundle config --local build.mysql2 --with-opt-dir="$(brew --prefix openssl)"
+bundle config --local build.local-fastimage_resize --with-opt-dir="$(brew --prefix gd)"


Looks like it works 👍🏻

In RHEL8+ ImageMagick is no longer provided due to being hard to maintain and prone to security issues. That's why we implement an image processor based on the `libgd` backed `local-fastimage_resize` gem for logo processing instead of the Paperclip's default ImageMagick based processor `Paperclip::Thumbnail`. We remove support for uploading GIF profile logos in the process. Logo is used when generating invoices. But prawn only supports JPEGs and PNGs. With ImageMagick we could specify target format for the `:invoice` style to be `png` regardless of source image format. `local-fastimage_resize` only resizes between same formats though. Provided very low adoption of GIF logos, it is not worth implementing such support for `local-fastimage_resize` although it shouldn't be too hard if need arise. Note: existing JPEG logos need to have the `:invoice` style regenerated with `rake fixes:regenerate_jpeg_invoice_logo`. fixes THREESCALE-8579 There is another part to this replacement and it is our gruff usage.

akostadinov self-assigned this Sep 15, 2022

akostadinov requested a review from a team September 15, 2022 18:20

akostadinov force-pushed the imagemagick branch 5 times, most recently from 34048fe to 1fc4cfe Compare September 16, 2022 15:37

jlledom previously approved these changes Sep 21, 2022

View reviewed changes

test/integration/provider/admin/account/logos_controller_test.rb Outdated Show resolved Hide resolved

josemigallas requested changes Sep 22, 2022

View reviewed changes

test/integration/provider/admin/account/logos_controller_test.rb Outdated Show resolved Hide resolved

INSTALL.md Outdated Show resolved Hide resolved

akostadinov dismissed jlledom’s stale review via f8f8615 September 22, 2022 17:21

akostadinov force-pushed the imagemagick branch 2 times, most recently from f8f8615 to db71c05 Compare September 22, 2022 17:42

akostadinov requested review from jlledom and josemigallas September 22, 2022 17:46

akostadinov force-pushed the imagemagick branch from db71c05 to d6a3fed Compare September 22, 2022 18:08

jlledom previously approved these changes Sep 23, 2022

View reviewed changes

test/unit/tasks/fixes_test.rb Show resolved Hide resolved

akostadinov dismissed jlledom’s stale review via ee446c2 September 23, 2022 11:57

akostadinov force-pushed the imagemagick branch from d6a3fed to ee446c2 Compare September 23, 2022 11:57

akostadinov requested a review from jlledom September 23, 2022 12:09

jlledom previously approved these changes Sep 23, 2022

View reviewed changes

mayorova reviewed Sep 23, 2022

View reviewed changes

app/models/profile.rb Outdated Show resolved Hide resolved

akostadinov dismissed jlledom’s stale review via a42a2fe September 23, 2022 15:42

akostadinov force-pushed the imagemagick branch from ee446c2 to a42a2fe Compare September 23, 2022 15:42

mayorova reviewed Sep 23, 2022

View reviewed changes

INSTALL.md Outdated Show resolved Hide resolved

mayorova reviewed Sep 24, 2022

View reviewed changes

nidhi-soni1104 reviewed Sep 26, 2022

View reviewed changes

akostadinov force-pushed the imagemagick branch from a42a2fe to 9607c8c Compare September 26, 2022 14:23

josemigallas changed the title ~~[DO NOT MERGE] use libgd instead of ImageMagick~~ 🚧 [DO NOT MERGE] use libgd instead of ImageMagick Nov 2, 2022

github-actions bot removed the Stale label Nov 3, 2022

github-actions bot added the Stale label Nov 18, 2022

github-actions bot closed this Nov 26, 2022

akostadinov mentioned this pull request Feb 20, 2023

THREESCALE-9162: Fix missing logo on invoices in SaaS #3194

Merged

akostadinov removed the Stale label Feb 28, 2023

akostadinov reopened this Feb 28, 2023

akostadinov changed the title ~~🚧 [DO NOT MERGE] use libgd instead of ImageMagick~~ use libgd instead of ImageMagick Feb 28, 2023

3scale deleted a comment from github-actions bot Mar 1, 2023

akostadinov mentioned this pull request Mar 2, 2023

📝 Update manual installation #3229

Merged

github-actions bot added the Stale label Mar 28, 2023

3scale deleted a comment from github-actions bot Mar 29, 2023

akostadinov removed the Stale label Mar 29, 2023

github-actions bot added the Stale label Apr 13, 2023

3scale deleted a comment from github-actions bot Apr 13, 2023

github-actions bot removed the Stale label Apr 14, 2023

akostadinov mentioned this pull request Apr 24, 2023

Upgrade Rmagick to 4.3.0 #3323

Closed

akostadinov dismissed stale reviews from jlledom, thalesmiguel, and mayorova via 148bf23 April 24, 2023 15:11

akostadinov force-pushed the imagemagick branch from 54d8018 to 148bf23 Compare April 24, 2023 15:11

josemigallas reviewed Apr 24, 2023

View reviewed changes

akostadinov force-pushed the imagemagick branch from 148bf23 to 08f6edf Compare April 24, 2023 17:24

akostadinov merged commit 1dbdb61 into 3scale:master Apr 24, 2023

akostadinov deleted the imagemagick branch April 24, 2023 17:55

	brew install chromedriver imagemagick@6 gs pkg-config openssl geckodriver sphinxgd [email protected] postgresql@14
	brew install chromedriver libgd gs pkg-config openssl geckodriver sphinxgd [email protected] postgresql@14

use libgd instead of ImageMagick #3065

use libgd instead of ImageMagick #3065

Uh oh!

Conversation

akostadinov commented Sep 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mayorova commented Sep 23, 2022

Uh oh!

Uh oh!

mayorova commented Sep 23, 2022

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mayorova commented Sep 24, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

josemigallas commented Nov 2, 2022

Uh oh!

thalesmiguel commented Feb 28, 2023

Uh oh!

josemigallas commented Mar 13, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

akostadinov commented Sep 15, 2022 •

edited

Loading