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

整理: requirements-license.txt を廃止 #1281

Merged
merged 17 commits into from
Jun 9, 2024

Conversation

tarepan
Copy link
Contributor

@tarepan tarepan commented May 19, 2024

内容

概要: requirements-license.txt を廃止してリファクタリング

現在の VOICEVOX ENGINE は依存パッケージを poetry で管理し、それを pip 用にエクスポートしている。
依存パッケージのライセンスリストを生成する際にはエクスポートされた requirements.txt が利用されるが、更にライセンス生成パッケージである pip-license が必要である。つまりライセンス生成には pip install -r requirements.txt && pip install pip-license==4.4.0 のコマンドが必要になる。
この手間を省くため、VOICEVOX ENGINE ではパッケージ group license を設定している。これは pip-license=^4.4.0 のみからなる group であり、エクスポートすると requirements.txt 内容 + pip-license が記録される。これを用いればライセンス生成時に pip install -r requirements-license.txt のみで完結する。

これは便利であるが、pip install pip-license==4.4.0 の記述を消すために requirements-license.txt という追加ファイルを生み出している。
このファイルの立ち位置を理解し、都度更新し、ビルド時の移動コマンドを正しく設定し、管理し続けるコストは低くない。
現状、記述削減メリットより管理コストが上回っていると考える(参照: 本 PR での削減量)。
すなわち、group license の仕組みは過剰である。

このような背景から、requirements-license.txt を廃止するリファクタリングを提案します。

本 PR では pip-license パッケージが poetry から外れるため、セキュリティ自動チェックの仕組みから外れることになる。これは安全性低下ではあるが、pip-license がライセンス生成のみ登場しビルド・プロダクションに登場しないことを鑑みて許容範囲と考える。

関連 Issue

無し

@tarepan tarepan requested a review from a team as a code owner May 19, 2024 10:08
@tarepan tarepan requested review from Hiroshiba and removed request for a team May 19, 2024 10:08
@Hiroshiba
Copy link
Member

完全な一長一短になっており、判断ができないなと感じました!

折衷案として、requirementsの内容をgrepしてインストールするのはどうでしょうか。
こんな感じになるかなと想像しています。

pip install <(cat requirements.txt | grep poetry)

@tarepan
Copy link
Contributor Author

tarepan commented May 23, 2024

requirementsの内容をgrepしてインストール
...
pip install <(cat requirements.txt | grep poetry)

意図を掴みそこねました🙇(bash ムズカシイ…🥺)
requirements.txt の中身は全てインストールされるべきなので、そこを grep でフィルタリングするというのはどういう感じでしょうか…?

@Hiroshiba
Copy link
Member

Hiroshiba commented May 27, 2024

ああすみません!
前提としてrequirements-dev.txt辺りにpip-licensesが入ってると思ってました!

まず-devpip-licensesを入れて、(でないとgenerate_licenses.pyのデバッグができない)
requirements-dev.txtからバージョン固定されたpip-licensesを持ってきてinstallするのが良いのかなぁと。
手元とCIで実行環境を揃えられるのと、CIコードに散らばった==4.4.0をなくせるメリットがありそうです。

前回のコメントはなぜかrequirements.txtからpoetryをgrepしてますね。。。 🙇
こんな感じかなぁ。

pip install <(cat requirements-dev.txt | grep pip-licenses)

@tarepan
Copy link
Contributor Author

tarepan commented May 27, 2024

👍️
理解しました。
こちらの方法で実装します。


@Hiroshiba
全指摘箇所の反映・テストパスを確認しました。Re-review よろしくお願いします。

Copy link
Member

@Hiroshiba Hiroshiba left a comment

Choose a reason for hiding this comment

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

LGTM!!

requirementsのtestがdevにリネームされた関係でコンフリクトが発生してしまったかもです 🙇

build_util/create_venv_and_generate_licenses.bash Outdated Show resolved Hide resolved
pyproject.toml Show resolved Hide resolved
@tarepan
Copy link
Contributor Author

tarepan commented Jun 2, 2024

@Hiroshiba
全指摘箇所の反映・テストパスを確認しました。Re2-review よろしくお願いします。

Copy link
Member

@Hiroshiba Hiroshiba left a comment

Choose a reason for hiding this comment

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

LGTM!!

おまたせしました!!
requirementsダルマ落とし、あと1段ですね!!!

@Hiroshiba Hiroshiba merged commit c88b3ad into VOICEVOX:master Jun 9, 2024
4 checks passed
@tarepan tarepan deleted the refactor/no_requirements_license branch June 9, 2024 09:41
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