[refactor] workspace単位で cargo 操作できるように workspace 分割

laysakura commented 3 years ago

現状、enclave, host の crate が同一 workspace 内に入り混じっており、feature flagの設定を考えると、トップレベルで workspace 単位で cargo build などしても失敗する。

これは

rust-analyzer の動作不全
CIにおける crate 単位でのclippy（crate追加の際に漏れがち）

などにつながっている。

_Originally posted by @laysakura in https://github.com/LayerXcom/anonify/pull/621#discussion_r645274052_

laysakura commented 3 years ago

serde-encrypt と serde-encrypt-sgx を作って得た知見

TL;DR

Rust SGX SDKでビルドするcrate（以下、「SGXなcrate」）はリポジトリを分けたほうが良い。ただし path 依存しかしない（git依存不要）な場合は同一リポジトリで cargo workspace から exclude する方式でも良い。
- いずれにせよSGX固有のコードはcrateが分かれるので、 sgx feature flag は不要。
「SGXなcrateから使われるcrate」は、 #![no_std] で作っておいて、Rust SGX SDKでのビルドもCIしておく。SGX以外からも使われるなら std feature flag を提供して #![cfg_attr(not(feature = "std"), no_std)] してもOK。

SGXなcrateを別リポジトリにすべき理由

SGXなcrateは、

serde-std = { package = "serde", version = "1", default-features = false, optional = true }
sgx_tstd = { rev = "v1.1.3", git = "https://github.com/apache/teaclave-sgx-sdk.git", optional = true }

みたいに、git 依存を含む。

git 依存（や path 依存）を含む crate は、crate.io にpublishすることができない。従って、SGXなcrateを他のクレートから depend してもらうためには、path依存またはgit依存してもらう必要がある。

git依存してもらうためには、独立したリポジトリにする必要がある。より正確に言うと、「リポジトリAがcargo workspaceで、そのメンバーとしてcrate Xを持っていたら、 package = "X", git = "A" のようにgit依存できる」という状況。

SGXなcrateをそうじゃないcrateと混ぜてworkspaceに入れておくととても大変なので、anonify-sgx というリポジトリでも作って、そこはSGXなcrateだけを含むcargo workspaceにするのが一番管理が楽ちんだと思う。

そもそもなぜSGXなcrateをそうじゃないcrate同じworkspaceに入れたくないか

workspaceのトップレベルで

cargo build

などすると、workspace内全crateに対してビルドが走る。これは本来とても嬉しい機能だが、SGXなクレートが混ざっていると、「SGXなクレートがビルドできる環境」じゃないと cargo build が正常終了しないことに繋がる。（rust-analyzerとかがうまく動かないのもこのあたりが原因だと思ってる）

開発速度を保つのに一番いいなと思うのは、

非SGXなcrateは素のmacOSでガンガンビルドもテストもrust-analyzerもできる
SGXなcrateは（今まで通り）SGXがビルドできる anonify-dev docker image みたいな環境でビルドし、SGXのランタイムがある環境でテストする

であり、そのためにはSGXなcrateは別workspaceに追いやる必要がある。

うま味

色々列挙できるけど、要は「SGXなcrate書いてるとき以外は普通のRust開発ができるようになる」です。

非SGXな箇所のテストとか気軽にかけるようになるし、コンパイルエラー駆動開発もやりやすい
CIのスクリプトが "普通" になる
- 普通に cargo test とかするだけでよくなる部分が増える
rust-analyzer使った開発もだいぶできるようになる
- 少なくとも非SGXなcrateが入ったrepoは確実に。SGXなcrateが入ったrepoも、anonify-devの中でrust-analyzer走らせればたぶん大丈夫

osuketh commented 3 years ago

加える論点としては、以下とかでしょうか。

sgx/stdにまたがるPRどうすべきか（std側のテストsgxに依存しpush&cargo updateしないとテスト試せないなど
sgx/stdが共通のコードベース使っている部分どうするか

laysakura commented 3 years ago

sgx/stdが共通のコードベース使っている部分どうするか

これは、共通のコードベースを非SGXなcrateとして

「SGXなcrateから使われるcrate」は、 #![no_std] で作っておいて、Rust SGX SDKでのビルドもCIしておく。SGX以外からも使われるなら std feature flag を提供して #![cfg_attr(not(feature = "std"), no_std)] してもOK。

の状態にしておくとよきだと思います。

sgx/stdにまたがるPRどうすべきか（std側のテストsgxに依存しpush&cargo updateしないとテスト試せないなど

まず、個人的にはsgxなクレートはだいぶ疎結合に作っておくべきだと思っています。sgxなクレートだけが入ったリポジトリで、結合試験まできっちりやる。その後で非sgxなクレートを書き始める、という形。

これが結構苦しいのであれば、git subtreeを使う形ならcargoのgit依存の制約も突破しつつ同一PRで一緒くたに見れるかもしれません。自信はない。

osuketh commented 3 years ago

azms!

「SGXなcrateから使われるcrate」は、 #![no_std] で作っておいて、Rust SGX SDKでのビルドもCIしておく。SGX以外からも使われるなら std feature flag を提供して #![cfg_attr(not(feature = "std"), no_std)] してもOK。

これは共通のコードベースでは、 no_std には対応していないが sgx_tstd では対応しているcrateは扱えない前提そうですね。frame周り多そうなのですが記憶が乏しくリファクタ進めていかねばわからず :gnn:

sgx/stdにまたがるPRどうすべきか

stdの結合テストではenclaveへの処理が依存してしまうので、enclave mock 入れるとかもあるのかな。 git subtree できたら良さそうですね〜

laysakura commented 3 years ago

あーstd使うかsgx_tstdを使うかでやってるってことですね。その場合は自分の提案的には SGXなクレートとして別リポに逃がすことになってしまい、非SGXなクレートからはかなり使いづらくなってしまいますね。実際にコード見て、本質的にはstd使わずに頑張れるものは頑張っていくという対応がありえますが、頑張れるものの割合次第ですね。幸い最近no_std職人力が上がってきたので、時間あるときに見てみます。

subtreeは新卒の職場でやってたのですが、git操作が複雑になりそれはそれでストレスだしオペミス増えるんですよね... やはりおすすめは、非SGXな部分は「SGXか何か知らんけどこういうインターフェイスで計算してくれるもの」ってノリのtraitに依存して、テストコードではmock実装する形です。

参考までに、serde-encrypt-core というクレートはSGXにも通常のserdeにも非依存で切り出していて、シリアライザブル的なtraitにめちゃ依存してます。そのtraitを実装しているのが、普通のserde依存の serde-encrypt.と、serde-sgx に依存した serde-encrypt-sgx です。

serde-encrypt-core 層のテストでは、新たにmockを作ることなく、serde-encrypt 層からテストしています。これはたまたま、非SGXなインプリがモックではなく直接ユースケース持っていたからですね。

LayerXcom / confidential-computing-modules