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

Bump rust-sgx-sdk to master #1133

Merged
merged 4 commits into from
Jan 3, 2023
Merged

Bump rust-sgx-sdk to master #1133

merged 4 commits into from
Jan 3, 2023

Conversation

Kailai-Wang
Copy link
Contributor

This PR tries to switch the rust-sgx-sdk from branch v1.1.6-testing to master so that it supports Intel SGX SDK 2.18.

Steps done:

  • make -f UpdateRustSGXSDK.mk
  • adjust TOML
  • bump pin-sgx
  • cargo update

First time bumping these versions, please let me know if I miss anything, thanks!

Copy link
Contributor

@clangenb clangenb left a comment

Choose a reason for hiding this comment

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

Nice, thanks a lot!

To me, this looks good except for the one duplicate dependency we have. Beware that the crypto helper is also used on the untrusted side, maybe this is where you have forgotten to change the dependency. But perhaps @haerdib can quickly confirm my judgement, as she did many of these updates.

@@ -1886,7 +1886,7 @@ dependencies = [
"parity-scale-codec",
"serde 1.0.118 (git+https://github.com/mesalock-linux/serde-sgx?tag=sgx_1.1.3)",
"serde_json 1.0.60 (git+https://github.com/mesalock-linux/serde-json-sgx?tag=sgx_1.1.3)",
"sgx_crypto_helper",
"sgx_crypto_helper 1.1.6 (git+https://github.com/apache/incubator-teaclave-sgx-sdk?branch=master)",
Copy link
Contributor

Choose a reason for hiding this comment

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

When you suddenly have additional information in the dep here, it means that we have the dependency twice. So we are probably still referencing this crate from the v1.1.6-testing branch somewhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should be resolved now

Comment on lines 3546 to 3560
source = "git+https://github.com/apache/incubator-teaclave-sgx-sdk?branch=master#27bd225ae6dbcd1d0a6d4d9590acc4d73c5195c2"

[[package]]
name = "sgx_crypto_helper"
version = "1.1.6"
source = "git+https://github.com/apache/incubator-teaclave-sgx-sdk?branch=master#27bd225ae6dbcd1d0a6d4d9590acc4d73c5195c2"
dependencies = [
"itertools",
"serde 1.0.118 (git+https://github.com/mesalock-linux/serde-sgx)",
"serde-big-array",
"serde_derive 1.0.118",
"sgx_tcrypto",
"sgx_tstd",
"sgx_types",
]
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, as you can see below, we still import the crypto helper from the testing branch too.

@clangenb clangenb added A0-core Affects a core part B1-releasenotes C1-low 📌 Does not elevate a release containing this beyond "low priority" E0-breaksnothing labels Jan 3, 2023
@Kailai-Wang
Copy link
Contributor Author

Good catch, thanks! 😂

Yes I missed core/tls-websocket-server/Cargo.toml where v1.1.6-testing branch was still used. It doesn't get properly overridden as the git URL doesn't match - I fixed that too

Copy link
Contributor

@clangenb clangenb left a comment

Choose a reason for hiding this comment

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

Wonderful, thanks for the prompt fix! I will merge as soon as the CI passes.

@clangenb clangenb merged commit fb13dcb into integritee-network:master Jan 3, 2023
@Kailai-Wang Kailai-Wang deleted the bump-rust-sgx-sdk-to-master branch January 3, 2023 11:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A0-core Affects a core part B1-releasenotes C1-low 📌 Does not elevate a release containing this beyond "low priority" E0-breaksnothing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants