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

Fix builds and add CI check for no-std targets #300

Closed
wants to merge 8 commits into from

Conversation

DrTobe
Copy link
Contributor

@DrTobe DrTobe commented Jul 18, 2023

The current master does not build for no_std targets because mbedtls-platform-supports uses once_cell::sync::OnceCell, the once_cell::sync module ist only available if the std feature is active for once_cell. Thus, the current master only builds for std targets.

This PR fixes this, but only if the tls13 feature is not active. (Edit: Not true anymore thanks to the critical-section feature in once_cell)

@Taowyoo Taowyoo self-requested a review July 18, 2023 20:00
Copy link
Collaborator

@Taowyoo Taowyoo left a comment

Choose a reason for hiding this comment

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

The PR LGTM.
But could you provide the build command that could reproduce the error.

If possible, need to add a build check on this in CI to avoid same problem in future

@DrTobe
Copy link
Contributor Author

DrTobe commented Jul 19, 2023

If possible, need to add a build check on this in CI to avoid same problem in future

That sounds like a great idea! I will try to add a corresponding build target in the CI when I find some time to work on that.

@DrTobe DrTobe force-pushed the optional-once_cell branch from 65c2c3b to 9665d52 Compare July 20, 2023 11:23
@DrTobe DrTobe force-pushed the optional-once_cell branch from 9665d52 to 74eeb22 Compare July 20, 2023 11:30
@DrTobe DrTobe force-pushed the optional-once_cell branch from 3d06edc to 5b949ee Compare July 20, 2023 16:21
@DrTobe DrTobe force-pushed the optional-once_cell branch from dbcd916 to bc0f3c4 Compare July 21, 2023 09:45
@DrTobe DrTobe changed the title Only use once_cell dependency if required to fix builds for no-std targets Fix builds and add CI check for no-std targets Jul 21, 2023
@DrTobe DrTobe marked this pull request as draft July 21, 2023 11:27
@DrTobe
Copy link
Contributor Author

DrTobe commented Jul 21, 2023

Fixing no_std support apparently requires more than I first anticipated. The current state is not safe to merge because many types impl Sync under the false assumption that mbedtls-sys is used with the "threading" feature (thus I converted this into "Draft" state). But before I can fix this I need to understand which parts needs to be made dependent on the "threading" feature and if more changes are necessary. This requires deeper understanding of the mbedtls codebase and unraveling some of the macro magic involved. In short, this will take some more time to be finished.

@xiaoyuxlu
Copy link
Contributor

@DrTobe @Taowyoo
I also have a problem with no_std build as well. #302
I suggest fixing no_std build first #303 . This will unblock the use of no_std.
The next step is to enable CI and tls13 in no_std. what do you think?

@Taowyoo
Copy link
Collaborator

Taowyoo commented Oct 19, 2023

Close this old PR because of change of default branch to main.
See more:

Please reopen this if have further problems or you rebased this PR onto main(this is needed on mbedtls 2.28.X)

@Taowyoo Taowyoo closed this Oct 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants