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

Move check coding rule setting file to C2A user top #97

Merged
merged 9 commits into from
Sep 21, 2023

Conversation

sksat
Copy link
Member

@sksat sksat commented Sep 21, 2023

概要

コーディングルールのスクリプトによるチェックを設定ファイル(check_coding_rule.json)があるディレクトリの配下に対して実施するように変更し,C2A user では check_coding_rule.json をトップディレクトリに置くように標準化します.

これにより,check_coding_rule.py の挙動が CWD によって変化するという非常に confusing な事態の根本的な解決をします.

Issue / PR

詳細

  • check_coding_rule.jsonc2a_root_dir を廃止した.理由は以下のようにあまりにも confusing なため
    • そもそも「C2A root dir」なる概念は undocumented である上に,C2A user のトップディレクトリではなく src/ のこと(であることを概ね期待している)
    • 設定ファイルが置かれる src/src_user/script/ci/ から src/ への相対パスであるかのように予想されるがそんなことは一切なく,概ね src/src_core/script/cicd して実行されることを期待した上でその実行パスからの相対パスになっている
    • この仕様はあまりにも分かりにくいため,上記の期待は一部の C2A user では守られていなかった
      • この問題は workflows-c2a によって全 C2A user で統一的な方法で check_coding_rule.py を実行するようになったことでようやく発覚・顕在化した
    • c2a-core では見た目上は C2A user (の src/src_user)と同じ場所である script/ci/ 配下に check_coding_rule.json が置かれていたが,ここではなぜか c2a_root_dir の意味が変わっていた(おそらく example user の src から実行することを期待している?)
  • <c2a-core>/script/ci/check_coding_rule.json を削除
    • 上記のように,なぜか C2A user とセマンティクスが変わって confusing であるため
    • また,c2a-core の開発においては,example user に対するチェックでワークするため
      • これについてはやり方を考えてもよいが,一旦は simple にすることを優先するため,別途やりたい
  • check_coding_rule.py のファイル探索範囲を check_coding_rule.json のあるディレクトリ以下に変更した
  • check_coding_rule.json を (c2a-core example user 含め)C2A user のトップディレクトリに置くように標準化する
    • これに伴い,設定ファイル中のディレクトリの設定は C2A user のトップディレクトリからの相対パスになる
    • これはほとんどの場合 src/src_user/*** になるはず

検証結果

workflows-c2a でのサポートは別途行うため,手元で python script/ci/check_coding_rule.py examples/mobc/check_coding_rule.json のように実行する

影響範囲

  • 全 C2A user のコーディングルールのチェック及びその設定ファイル
  • workflows-c2a

補足

@sksat sksat added enhancement New feature or request priority::high priorityg high tools labels Sep 21, 2023
@sksat sksat self-assigned this Sep 21, 2023
@meltingrabbit
Copy link
Member

コーディングルールのスクリプトによるチェックを設定ファイル(check_coding_rule.json)があるディレクトリの配下に対して実施するように変更し,C2A user では check_coding_rule.json をトップディレクトリに置くように標準化します.

user/settings/ or user/script/ などではなくトップがいい理由ってなんでしたっけ?
トップにごちゃごちゃ増えていくのはあまり好きではない,というだけなんだけど.

@sksat
Copy link
Member Author

sksat commented Sep 21, 2023

トップディレクトリがごちゃごちゃになるのは僕もそんなに好きではないですが,今よりははるかにマシ,という気持ちです

@meltingrabbit
Copy link
Member

であれば,

  1. トップディレクトリ
  2. user/settings/
  3. user/script/

で比較かなと思いますが,その中でトップディレクトリ選んだのはなぜです?(個人的には 2. でよくない?とおもったり)

@sksat
Copy link
Member Author

sksat commented Sep 21, 2023

src/src_coresrc/src_user の対比構造を(言ってしまえばやや雑に)敷いてしまったことによって,src/src_user/scriptsrc/src_core/script に設定ファイルがある,みたいな歪な構造が生まれてしまっていた,というのがこの問題だと思うので,src/src_user/script には置きたくないです.user にあるのは設定ファイルであってスクリプトでは一切ないですし......
また,このスクリプトはそもそも適宜ローカルでも実行すべきものだと思うので,CI みたいなディレクトリには置きたくないです(この PR とは別に src/src_core/Script/CI を廃止する変更もこの後別途やろうと思っています).

src/src_user/settingssrc/src_user/script よりはアリですが,それを言うのであれば satconfig.json などについても議論すべきというのが1割,そんなことをしたらどうせ相対パスで遡るやつやパスの非自明が多発するのでそんなことをするぐらいならトップディレクトリに置く方が遥にマシ,が9割,というかんじです.

@sksat sksat force-pushed the feature/refactor-check-coding-rule-path branch from 68f1dbb to 24301d4 Compare September 21, 2023 07:37
@tarotene
Copy link
Contributor

普通のフォーマッタに期待したい挙動としては

  • 設定ファイルが実行元(cwd)に置かれていればそれを参照し,なければ何らかのデフォルト設定を利用する
  • その気になれば設定ファイルをカスケードっぽく階層的に配置することもできる

だと思うので(この想定が標準的ではない,もしくは現状の運用と著しく異なりナンセンス,などのツッコミはあると思います),そこと衝突しそうなところについて質問します.


  • 下記の理由について教えてください:

    check_coding_rule.py のファイル探索範囲を check_coding_rule.json のあるディレクトリ以下に変更した

    例えば,実行元が常に repository root (雑には git rev-parse --show-toplevel の戻り値)であることをユーザに要求する(≒ そうでない場合 strict に未定義な挙動であることを受け入れてもらう)のであれば,例えば

    • 実行元(cwd)以下
    • check_coding_rule.py のあるディレクトリ以下
    • その他何らかの固定的なディレクトリ以下(src/src_user, src/src_core に絞る,とか)

    なども選択肢に入ってくると思います.

  • 諸々の問題は check_coding_rule.json に探索範囲(適用されたいファイル),適用したいルールその他からなる複数の直交する情報が書かれていることも原因の一つだと認識しているのですが,それを分割するという戦略は今後実施しないと思って良いですか?要するに下記の「標準化」のニュアンスを知りたいです:

    check_coding_rule.json を (c2a-core example user 含め)C2A user のトップディレクトリに置くように標準化する

    例えば自分なら,オプション無しの引数にファイル名を取りたいな,とか,設定ファイルはオプション付きにしたいな,とかを考えて,これを段階的にほぼ無痛でやれる方法を考えるかなと思います(良し悪しは置いておいて/おそらく考慮済みではあると思います).どうせ種々の c2a-user に対して breaking なことをやるのであれば,much more breaking でも良いのではという気持ちがあります.

@sksat
Copy link
Member Author

sksat commented Sep 21, 2023

ファイル探索範囲のルートディレクトリと「標準化」については関連はするものの別種の要求となります.

前者については,これはそもそも repository root のような定まったディレクトリではなく,strict にはあくまで(文字通り)指定された設定ファイルのある場所以下になります.つまり,python src/src_core/script/ci/check_coding_rule.py /tmp/hoge-c2a-user/check_coding_rule.json のように実行した時の探索範囲は /tmp/hoge-c2a-user/ 以下になります.

この時,実行時の CWD に関する要求は特にしていません.これにより,例えば今までできなかった

cd <c2a-user>
cd src/src_user
# なんらかの作業
python ../src_core/script/ci/check_coding_rule.py ../../check_coding_rule.json

のような動作が可能になります.まあこれは少し面倒なのであんまりやらない操作だとは思いますが,このように実行したとしても非自明な結果(なにもチェックできていなかった,など)にならない,ということをこの変更では重視しています.
実行時の CWD 以下でチェックされる方が自明という意見もあるかもしれませんが,設定ファイルを明示的に指定して走らせている以上,その設定ファイルがあるディレクトリ以下が対象になることはそこまで非自明ではないかな,という認識です.

また,check_coding_rule.py のあるディレクトリ以下,は単に意味がありません.このファイルは c2a-core にあり,(その内部で移動したとしても)C2A user では <repository root>/src/src_core 以下になるからです.

固定的なディレクトリを対象にしなかったのは,少なくとも直近ではおそらくそれがワークしない(し,するように運用を変えるとしてもそれは v4.0.0 の後に実施すべきだと考えている)からです.
結局 ignore_files などを C2A user 毎に設定しないといけない現状に対して早急すぎるかな,という感覚です.
c2a-aobc などを中心に,そもそも現状の check coding rule スクリプトがあまりちゃんと走っていなかった,という場もあるわけなので,まずはスクリプトがどの C2A user でもちゃんと走るようにして,そして AE 外含め全 C2A user で workflows-c2a を使ってもらうなどしてその状態を維持するのが先決です.

後者の「標準化」についてですが,これは「設定ファイルのあるディレクトリ以下でチェックする」という挙動の変更に合わせて,追加の要求(というか recommendation)として「設定ファイルを C2A user のトップディレクトリに置いてほしい」というお願いをしたい,というものです.このお願いの手段が release note や CHANGELOG.md では不十分では,みたいな指摘はあると思っているので,そこについては(この PR とは別に)意見欲しいです.
このお願いの目的は,どの C2A user でも同じ開発プロセスをとっていてほしくて,その一環として同じディレクトリ・ファイルパス構成でコーディングルールのチェックが同じように行われていてほしいからです.
これを workflows-c2a のデフォルト設定としておくと,この「標準」に則っていれば真に1行で CI が導入できる,という状態も維持できます.

much more breaking にしてもいいのでは,については,ナシではないけれどそこまでメリットが無いかな,という認識です.例えばファイルを入力に取るようにする,などは clang-format のように動いてほしい,ということだと思うのですが,その場合は(まさに clang-format のように)1ファイルを対象にフォーマットを実施するコマンドと,そのコマンドをプロジェクトに対して走らせるスクリプト(find してループする,みたいなやつ)に実装を分割すべきだと思います.コマンドの入力の型が変わってしまうことになるので.そして,そのような改修を行う場合には,この変更の後に無痛で改修を実施できると思います(仮にやるとしても別 PR だし,ここでの方針を変えるようなものではない).というようなことは思うものの,1ファイルに対してこのチェックを走らせたいことがそこまであるかというとかなり疑問です.実際 clang-format の挙動は不便に感じたことの方が明らかに多いです.

あと,CWD 以下を探索範囲にしてもよいのでは,というのも気持ちは分かるんですが,それをやるには無視したいファイルやディレクトリがありがちだし,.gitignore のようなモノは発生させたくない(設定ファイルを作るならプロジェクト毎にまとめたい)し,そもそも環境に対してインストールされているわけでもないのでちまちま走らせるのは面倒(python ../../../../../src_core/script/ci/check_coding_rule.py したいですか?)だし,そもそもこのルールは適用対象の全ファイルで維持されていて欲しいやつなのだから,最大のユースケースは全部チェックして違反してるやつを上から全部直していく,だし,割に合わないかな,と思っています.
頻繁に走らせるには(内容に対して)不当に遅いので並列化するなり多少の高速化は試みてもいいかな,とは思いましたが.

@sksat
Copy link
Member Author

sksat commented Sep 21, 2023

まあ,走らせるのが面倒,に関しては npm script 経由にして npm run lint:check-coding-rule とかやるのはナシではないかもしれない

@sksat
Copy link
Member Author

sksat commented Sep 21, 2023

あ,あと一番大きいのは check_coding_rule.py は formatter ではなく,format について怒ってくる linter でしかない,というのがあると思います.自力でフォーマットするなら(commit 分けるためとかで)個々のファイルについて走らせたいことはありがちなんですが,auto-fix しない/できない以上ファイル毎に走らせたい理由は速度くらいしかないのでは,という感覚です.

@tarotene
Copy link
Contributor

tarotene commented Sep 21, 2023

諸々,了解です.
(疑問は解決されました)

また,check_coding_rule.py のあるディレクトリ以下,は単に意味がありません.

これは,src_user と src_core が別であること考えたらそうですね...(つい忘れがちになる)

@sksat
Copy link
Member Author

sksat commented Sep 21, 2023

reviewdog のテストしとくか

@@ -9,6 +9,7 @@
#ifdef DEFINE_MAIN_ON_SILS
int main(void);
#endif

Choose a reason for hiding this comment

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

🚫 [check_coding_rule] reported by reviewdog 🐶
ANY SPACE AT EOL NEEDS TO BE REMOVED

Copy link
Member Author

Choose a reason for hiding this comment

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

OK

@meltingrabbit
Copy link
Member

よさそう.
CIとおるようにしておく(check cording rule wf を c2a-workflowに変更)のお願いしたい

@sksat
Copy link
Member Author

sksat commented Sep 21, 2023

workflows-c2a 側の対応をマージ・リリースしたので切り替えます
https://github.com/arkedge/workflows-c2a/releases/tag/v4.3.0

@sksat
Copy link
Member Author

sksat commented Sep 21, 2023

@meltingrabbit 切り替えました.ちゃんと走ってるので approve 欲しいです

@sksat sksat merged commit 775f41d into develop Sep 21, 2023
34 checks passed
@sksat sksat deleted the feature/refactor-check-coding-rule-path branch September 21, 2023 14:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request priority::high priorityg high tools
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants