Skip to content

フィーチャーverify-mr-enclave-enableにおける抜け漏れ対応 - #657

Merged
jkcomment merged 26 commits into
mainfrom
fix-verify-mr-enclave-enable-feature
Sep 8, 2021
Merged

フィーチャーverify-mr-enclave-enableにおける抜け漏れ対応#657
jkcomment merged 26 commits into
mainfrom
fix-verify-mr-enclave-enable-feature

Conversation

@jkcomment

@jkcomment jkcomment commented Sep 6, 2021

Copy link
Copy Markdown
Contributor

Issueへのリンク

  • なし

やったこと

  • verify-mr-enclave-enableの抜け漏れ対応

やらないこと

  • なし

動作検証

  • 別のリポジトリにて動作確認済

参考

  • なし

@jkcomment
jkcomment requested a review from cipepser September 6, 2021 11:52
cipepser
cipepser reviewed Sep 6, 2021
View reviewed changes

@cipepser cipepser left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

default-featuresがfalseじゃないときにビルドできない気がしました。
例えば、frame-enclaveではframe-runtimeが常にdefault-features = falseになってしまっているので、いざverify-mr-enclave-enableを有効にしたいときに
frame-enclave→verify-mr-enclave-enable有効
frame-runtime→verify-mr-enclave-enable無効
となって整合性が取れなくなってしまうかと。

具体的な解決策としては、frame-enclaveのCargo.tomlを以下のようにする感じですかね〜

verify-mr-enclave-enable = [
   "frame-runtime/verify-mr-enclave-enable"
]

※frame-enclave以外もあるので、全体的に整合性取る必要あり

@jkcomment

jkcomment commented Sep 7, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

確かに例のあのリポジトリの方では下記のような形でOn/Offをしてました

Cargo.toml

[features]
default = ["backup-enable", "runtime_enabled", "verify-mr-enclave-enable"]
backup-enable = [
  "anonify-enclave/backup-enable",
  "frame-runtime/backup-enable",
]
runtime_enabled = []
verify-mr-enclave-enable = [
  "anonify-enclave/verify-mr-enclave-enable",
  "frame-runtime/verify-mr-enclave-enable",
]

@jkcomment
jkcomment requested a review from cipepser September 8, 2021 00:18
cipepser
cipepser reviewed Sep 8, 2021
View reviewed changes

@cipepser cipepser left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

質問

  • tests/integration以下はfeaturesを設定しない設計ですかね？

対応漏れ

  • example/encrypted-sql-ops/enclave/Cargo.toml
  • modules/encrypted-sql-ops-enclave/Cargo.toml
  • tests.shのmakeでFEATURE_FLAGS

Comment thread frame/treekem/Cargo.toml
frame-config = { path = "../config", default-features = false, features = ["sgx"] }
frame-common = { path = "../common", default-features = false, features = ["sgx"] }
frame-mra-tls = { path = "../../frame/mra-tls" }
frame-mra-tls = { path = "../../frame/mra-tls", default-features = false }

@cipepser cipepser Sep 8, 2021

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

features に /verify-mr-enclave-enable が必要そう

@cipepser cipepser Sep 8, 2021

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

こっちは対応する（ハドルでやった）

@jkcomment jkcomment Sep 8, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

:done2:

Comment thread frame/runtime/Cargo.toml
Comment on lines 11 to +12
frame-treekem = { path = "../treekem", default-features = false, optional = true }
frame-mra-tls = { path = "../mra-tls", optional = true }
frame-mra-tls = { path = "../mra-tls", default-features = false, optional = true }

@cipepser cipepser Sep 8, 2021

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

treekemとmra-tls、featuresに設定必要ですね

@cipepser cipepser Sep 8, 2021

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

対応不要

@jkcomment

jkcomment commented Sep 8, 2021

Copy link
Copy Markdown
Contributor Author

tests/integration以下はfeaturesを設定しない設計ですかね？

tests/integrationの方は元々backup-enableも設定されていないのでそういう意図かなと思い、特に対応はしていない感じです。
もしfeaturesを設定するとしたらbackup-enableも一緒にやった方がいいですかね

@jkcomment
jkcomment requested a review from cipepser September 8, 2021 05:38
cipepser
cipepser approved these changes Sep 8, 2021
View reviewed changes

@cipepser cipepser left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

お疲れ様でした！

@jkcomment
jkcomment merged commit 735b067 into main Sep 8, 2021
@jkcomment
jkcomment deleted the fix-verify-mr-enclave-enable-feature branch September 8, 2021 05:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cipepser cipepser cipepser approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@jkcomment @cipepser