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

追加: actionlint CI #1124

Merged
merged 14 commits into from
May 17, 2024
Merged

追加: actionlint CI #1124

merged 14 commits into from
May 17, 2024

Conversation

tarepan
Copy link
Contributor

@tarepan tarepan commented Mar 18, 2024

内容

actionlint を CI に追加し GitHub Actions workflow の自動 linting を追加した。

ShellCheck 未導入のため、actionlint-ShellCheck 連携は現段階で導入していない。

関連 Issue

resolve #525 (final step 🎉)

@tarepan tarepan requested a review from a team as a code owner March 18, 2024 03:51
@tarepan tarepan requested review from y-chan and removed request for a team March 18, 2024 03:51
@tarepan tarepan marked this pull request as draft March 18, 2024 03:59
@Hiroshiba
Copy link
Member

シェルスクリプトはなかなか記法が難しいので、もしどうすれば良いかわからないのあればお聞きください!
(自分もわからないかもですが、誰がわかるかもしれない)

@tarepan
Copy link
Contributor Author

tarepan commented May 6, 2024

残エラー

パスの変更

エラー: .github/workflows/build-engine-package.yml:151:9: shellcheck reported issue in this script: SC1003:info:3:72: Want to escape a single quote? echo 'This is how it'\''s done' [shellcheck]

該当箇所:

CUDA_ROOT=$( echo "${{ steps.cuda-toolkit.outputs.CUDA_PATH }}" | tr '\\' '/' )

CUDAのセットアップ出力であるパスを編集している模様。期待出力がわからず手を入れづらい。

変数ぽい何か

エラー: .github/workflows/build-engine-package.yml:525:9: shellcheck reported issue in this script: SC2016:info:4:22: Expressions don't expand in single quotes, use double quotes for that [shellcheck]

該当箇所:

patchelf --set-rpath '$ORIGIN' "$(pwd)/download/onnxruntime/lib"/libonnxruntime_providers_*.so

$ORIGIN が指している対象がわからない。変数ではなく内部コマンド的な何か?

cat

エラー: .github/workflows/test-engine-package.yml:72:9: shellcheck reported issue in this script: SC2002:style:3:5: Useless cat. Consider 'cmd < file | ..' or 'cmd file | ..' instead [shellcheck]

該当箇所:

cat "download/list.txt" | xargs -I '%' curl -L -o "download/%" "${{ steps.vars.outputs.release_url }}/%"

私のシェル力が足りないため正解がよくわからない。

@yamachu
Copy link
Member

yamachu commented May 6, 2024

CUDA_ROOT=$( echo "${{ steps.cuda-toolkit.outputs.CUDA_PATH }}" | tr '\\' '/' ) 

ここに関しては Windows 対応のためかなと思います
Windows 環境での CI の場合、ファイルパスに C:\\Program Files みたいのが入ってると思うので、それを C:/Program Files みたいなパスにしてるはずです

修正は…これは

- tr '\\' '/'
+ tr "\\" "/"

でなんとかなりませんかね…?

'$ORIGIN' は rpath のエントリ名のはずですね
shell 変数ではないので、ダブルクオートで囲むでいいと思います

 cat "download/list.txt" | xargs -I '%' curl -L -o "download/%" "${{ steps.vars.outputs.release_url }}/%" 

これは download/list.txt を1行ずつ読み取り、curl でダウンロードしています
例えば download/list.txt に以下のような内容が含まれていたら

foo.zip
bar.zip

${{ steps.vars.outputs.release_url }}/foo.zip からダウンロードしたものを download/foo.zip として保存するみたいな感じです

修正案としては

<download/list.txt xargs -I '%' curl -L -o "download/%" "${{ steps.vars.outputs.release_url }}/%" 

とかでしょうか

@sevenc-nanashi
Copy link
Member

sevenc-nanashi commented May 6, 2024

'$ORIGIN' は rpath のエントリ名のはずですね
shell 変数ではないので、ダブルクオートで囲むでいいと思います

これダブルクォートで囲んじゃ駄目な様な。多分Shellcheckのignoreが正解だと思います。サイト見ても「この時はignoreしてね」って書いてありますし

https://www.shellcheck.net/wiki/SC2016

@yamachu
Copy link
Member

yamachu commented May 6, 2024

あぁ、そうか
その場で展開されて(おそらく)空文字になってしまいますね、ありがとうございます

@tarepan
Copy link
Contributor Author

tarepan commented May 7, 2024

'$ORIGIN' は rpath のエントリ名

Shellcheckのignoreが正解

@yamachu @sevenc-nanashi
ありがとうございます!
RPATH の特殊トークンであり、シェル変数化しないようにシングルクォートでエスケープすべきということですね。
Shellcheck は false alert ということで ignore します。

@tarepan
Copy link
Contributor Author

tarepan commented May 7, 2024

パスの変更

OS ごとの差異吸収とのことだったので GitHub Actions を用いて検証しました。

  • CUDA_PATH
    • Windows NVIDIA GPU: C:\Program Files\NVIDIA GPU Computing Toolkit\CUDA\v11.8
    • Linux NVIDIA GPU: /usr/local/cuda-11.8
  • CUDA_ROOT
    • Windows NVIDIA GPU: C:/Program Files/NVIDIA GPU Computing Toolkit/CUDA/v11.8
    • Linux NVIDIA GPU: /usr/local/cuda-11.8

@yamachu さんの指摘通り、パス区切りの OS 差異吸収として機能しているようです。アドバイスありがとうございます!

echo "C:\Program Files\NVIDIA GPU Computing Toolkit\CUDA\v11.8" | tr "\\" "/" を実行すると tr: warning: an unescaped backslash at end of string is not portable という警告がでました。シングルクォートは避けられ無さそうなので shellcheck ignore します。

@tarepan tarepan marked this pull request as ready for review May 14, 2024 07:54
@tarepan tarepan requested a review from a team as a code owner May 14, 2024 07:54
@tarepan tarepan removed the request for review from a team May 14, 2024 07:54
@tarepan tarepan requested a review from Hiroshiba May 14, 2024 07:54
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!!!

maxdepthの方だけ変更適用させていただきます!

いや~~~~~~~~~~~~~~~~~~~~bashあまりにも難しいですね!!!!!!!!

ちょっと全然関係ないのですが、Deno+zxにしようというのをなんとなく検討していたりするのですが、どう思われますか・・・・・?

Comment on lines +532 to 534
# NOTE: `$ORIGIN` は RPATH の特殊トークンであるため、bash 変数扱いされないために適切なエスケープが必要。
# shellcheck disable=SC2016
patchelf --set-rpath '$ORIGIN' "$(pwd)/download/onnxruntime/lib"/libonnxruntime_providers_*.so
Copy link
Member

Choose a reason for hiding this comment

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

一応こうやると迂回できそうでした。どっちがいいですかね。。。

Suggested change
# NOTE: `$ORIGIN` は RPATH の特殊トークンであるため、bash 変数扱いされないために適切なエスケープが必要。
# shellcheck disable=SC2016
patchelf --set-rpath '$ORIGIN' "$(pwd)/download/onnxruntime/lib"/libonnxruntime_providers_*.so
patchelf --set-rpath "\$ORIGIN" "$(pwd)/download/onnxruntime/lib"/libonnxruntime_providers_*.so

Copy link
Contributor Author

Choose a reason for hiding this comment

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

円マークをエスケープする、なるほどその手が。
提案頂いた方がスマートに感じます。

.github/workflows/build-engine-package.yml Outdated Show resolved Hide resolved
.github/workflows/test-engine-package.yml Show resolved Hide resolved
@Hiroshiba Hiroshiba merged commit 856a117 into VOICEVOX:master May 17, 2024
4 checks passed
@tarepan
Copy link
Contributor Author

tarepan commented May 17, 2024

Deno+zxにしようというのをなんとなく検討していたりするのですが、どう思われますか

(Deno+zx を使ったことがないのであくまで直感です)

ENGINE への導入はなかなかメンテ性に課題あり、が第一感です。

Deno ランタイムと zx ライブラリの安定性もそうですが、Python レポジトリに JS/TS をガンガン入れていくとメンテ(実装/改変/レビュー)できる人が限られそうな予感はします。エディタでは全然ありだと思います。
bash 辛いは本当に本当にそうなので、ENGINE で Deno+zx にコスト掛けれるならいっそ Python スクリプト化を目指すほうが安牌な気がしなくもなくもないです。

@tarepan tarepan deleted the add/actionlint branch May 17, 2024 18:18
@Hiroshiba
Copy link
Member

Deno+rxは使ってみたのですが、結構ハードルは高いように感じました。
以前ほど積極的に使いたいとは感じなくなりました。

かといってpythonも、とくにコマンド実行が面倒なのが厄介な気もしますね・・・。
なんだかんだbashなんですかねー・・・

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.

ShellCheckとactionlintを導入する
4 participants