Return data correctly for webp requests #72
Conversation
fukajun
commented
Jun 29, 2021
•
fukajun
commented
- Golang version up to 1.16.5.
- Update the image magick version to 6.9.12-17. For update version to latest.
- Fix compile kinu.
- Return webp data correctly. Because Currently return jpg for webp request.
- Use go module to resolve dependency.
handler_sandbox.go
Outdated
| if err != nil { | ||
| RespondInternalServerError(w, err) | ||
| return | ||
| } |
There was a problem hiding this comment.
関数定義が変更されてerr自体を返さなくなったみたいです
https://github.com/satori/go.uuid/blob/b2ce2384e17bbe0c6d34077efa39dbab3e09123b/generator.go#L68
There was a problem hiding this comment.
いやなんか勘違いしてるかもしれない。もう一度確認してみますね。
There was a problem hiding this comment.
https://github.com/satori/go.uuid/blob/v1.2.0/generator.go#L64 v1.2.0を利用しているのでコチラのURLが正しいものです。
go moduleによる管理に切り替えたときにgo.uuidは master から v1.2.0 を利用するように変わったみたいです。そのときに、errorを返さない実装に変わったため修正したという流れです。 @0tany
There was a problem hiding this comment.
現在使用しているバージョン、v1.2.0★では、err返却はなく、nilにもならない★の代わりに panic が内部的に起きるみたいです。(コードから推測する挙動が間違ってたらすみません。)
これは、次のリリースあたりで挙動が変更されるみたい(★)で、panicを使わない実装になるみたいです。
指摘のリスク自体はないみたいですが、別バージョンを使ったほうが良いのでしょうか？ @takatoshi-maeda
There was a problem hiding this comment.
差し替えのコストがさほどないなら、差し替えてしまったほうが良いと思います。確率はとても低く無視もできる範囲だとは思いますが、panic起こすとプロセス落ちちゃうので
| ENV IMAGE_MAGICK_VERSION=6.9.12-17 | ||
| RUN wget https://download.imagemagick.org/ImageMagick/download/releases/ImageMagick-${IMAGE_MAGICK_VERSION}.tar.gz && \ | ||
| tar xvzf ImageMagick-${IMAGE_MAGICK_VERSION}.tar.gz && \ | ||
| cd ImageMagick-${IMAGE_MAGICK_VERSION} && ./configure && make && make install && ldconfig && \ |
There was a problem hiding this comment.
旧版と新版で結構 imagemagick の Delegates が違いそう(./configure に与えるオプションが違ってそう)ですが、kinu から呼ぶ時に特に関係ない落としても問題ない機能が落ちてるんですかね？
あたりが気になりました。
// 旧kinuコンテナ
root@7be10b94d8b8:/# convert --version
Version: ImageMagick 6.9.6-2 Q16 x86_64 2016-10-13 http://www.imagemagick.org
Copyright: Copyright (C) 1999-2016 ImageMagick Studio LLC
License: http://www.imagemagick.org/script/license.php
Features: Cipher DPC Modules
Delegates (built-in): bzlib djvu fftw fontconfig freetype jbig jng jpeg lcms lqr ltdl lzma openexr pangocairo png tiff webp wmf x xml zlib
// 新kinuコンテナ
root@07965b2cb0b3:/kinu-build# convert --version
Version: ImageMagick 6.9.12-17 Q16 x86_64 2021-06-25 https://imagemagick.org
Copyright: (C) 1999-2021 ImageMagick Studio LLC
License: https://imagemagick.org/script/license.php
Features: Cipher DPC OpenMP(4.5)
Delegates (built-in): jng jpeg png webp zlib
There was a problem hiding this comment.
gif, png, jpeg, webpのみ必要なのでインストールするパッケージを絞りました。
旧版ではaptで libmagickwand-dev をインストールしていたので関連パッケージも一緒にインストールされて
色々なフォーマットをサポートしてたみたいです。
今回特定バージョンのImageMagickを使うためにソースから作ることにしたので、libmagickwand-devは
不要になりましたが、削除すると関連するlibjpg, libpng なども入らなくなるため
必要なフォーマットライブラリのみインストールする方針にしました。
途中、関連するフォーマットライブラリがほしかったのでlibmagickwand-devをapt installした後にapt removeするとかしてたけどイマイチなのと、docker imageサイズを小さくしたかったのでやめました。
|
gz形式のやつがなくなっていたので、マイナーバージョンだけ上げたものを使うようにした。 |
|
前回のレビューから 7f05137 以降のコミットで変更しているので、再レビューお願いします。 |
go.mod
Outdated
| gopkg.in/gographics/imagick.v2 v2.6.0 // indirect | ||
| ) | ||
|
|
||
| replace github.com/tokubai/kinu => ./ |
There was a problem hiding this comment.
https://go.dev/doc/modules/managing-dependencies#unpublished
ユースケース的に問題なさそうなのでこのままで 🙏
There was a problem hiding this comment.
自分自身がgithub.com/tokubai/kinuであるということを書けば、replaceは必要なかった
7ab096e
| @@ -1,19 +1,16 @@ | |||
| module kinu | |||
| module github.com/tokubai/kinu | |||
There was a problem hiding this comment.
自分自身のmodule名を正しくして、replaceを削除しました。
Dockerfile
Outdated
| ENV GOROOT /usr/local/go | ||
| ENV GOPATH /usr/local/go/vendor | ||
|
|
||
| ENV KINU_VERSION 1.0.0.alpha13 | ||
| ENV KINU_VERSION 1.0.0.alpha14 |
There was a problem hiding this comment.
僕が残した残骸ですが、 1.0.0 にしても良さそう...
handler_sandbox.go
Outdated
| if err != nil { | ||
| RespondInternalServerError(w, err) | ||
| return | ||
| } |
|
#72 (comment) について返信しました |
fukajun
requested review from
takatoshi-maeda
and removed request for
takatoshi-maeda
