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.

Sign up for GitHub

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

Jump to bottom

Update to newest version of rust-mbedtls #2962

Merged
merged 8 commits into from
Jan 24, 2023
Merged

Update to newest version of rust-mbedtls #2962

merged 8 commits into from
Jan 24, 2023

Conversation

the-real-adammork
Copy link
Contributor

Update to newest version of rust-mbedtls. Use heapless 0.8 to solve dependency issue where newer mbedtls needs spin 0.9.4.

Motivation

Some assembly code from MbedTLS would break when compiled with "newer" versions of clang bundled with Xcode 13/14. The upstream fix was merged to rust-mbedtls this PR brings that change to mobilecoin

Future Work

  • Bring change to libmobilecoin

i still make hiphop lol - lvusm

@the-real-adammork the-real-adammork mentioned this pull request Dec 15, 2022
Closed
@the-real-adammork the-real-adammork requested review from eranrund, cbeck88 and jcape and removed request for eranrund and cbeck88 December 15, 2022 19:30
@github-actions
Copy link

❌ Heads up, I tried building android-bindings using this branch and it failed.
Build logs can be found by inspecting the github action run Check that repositories submoduling us will still build after this PR / android-bindings or by clicking here.

eranrund
eranrund approved these changes Dec 15, 2022
View reviewed changes
Copy link
Contributor

@eranrund eranrund left a comment

Choose a reason for hiding this comment

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

LGTM, based on #2962 (comment), looks like once it gets merged you should go to https://github.com/mobilecoinofficial/android-bindings/blob/main/Cargo.toml (and likely full-service too?) to update the mbedtls version
Seems like at least with android-bindings it should be an easy uprev.

eranrund
eranrund suggested changes Dec 15, 2022
View reviewed changes
util/vec-map/Cargo.toml Outdated Show resolved Hide resolved
@github-actions
Copy link

✅ Good job, android-bindings was built successfully.
Build logs can be found by inspecting the github action run Check that repositories submoduling us will still build after this PR / android-bindings or by clicking here.

@the-real-adammork the-real-adammork force-pushed the adam/update-mobilecoin-repo-with-mbedtls-fix-simple branch from 5f9bb88 to 95520c4 Compare January 6, 2023 05:49
jcape
jcape approved these changes Jan 11, 2023
View reviewed changes
Copy link
Contributor

@jcape jcape left a comment

Choose a reason for hiding this comment

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

LGTM with some nitpicks

util/vec-map/Cargo.toml Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
the-real-adammork and others added 2 commits January 11, 2023 17:20
@the-real-adammork
Update util/vec-map/Cargo.toml
3298fd5 
Co-authored-by: James Cape <james@mobilecoin.com>
@the-real-adammork
Update Cargo.toml
8df42de 
Co-authored-by: James Cape <james@mobilecoin.com>
@the-real-adammork
Copy link
Contributor Author

@jcape thank you for the suggestions, I accepted both. Also left a new comment on a older thread with the error output for @eranrund 's question.

eranrund
eranrund approved these changes Jan 12, 2023
View reviewed changes
Copy link
Contributor

@eranrund eranrund left a comment

Choose a reason for hiding this comment

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

Suggest submitting a PR to https://github.com/japaric/heapless with the spin update, it appears actively maintained.

@the-real-adammork
Copy link
Contributor Author

the-real-adammork commented Jan 12, 2023 •
edited
Loading

  • heapless PR
  • Updated rust-mbedtls to the latest commit hash on mc-develop

@briancorbin briancorbin enabled auto-merge (squash) January 24, 2023 20:10
@briancorbin briancorbin merged commit b8d4331 into master Jan 24, 2023
@briancorbin briancorbin deleted the adam/update-mobilecoin-repo-with-mbedtls-fix-simple branch January 24, 2023 20:16
@jcape jcape mentioned this pull request Feb 3, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jcape jcape jcape approved these changes

@eranrund eranrund eranrund 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.

None yet
4 participants
@the-real-adammork @jcape @eranrund @briancorbin