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

Making core no_std compatible #94

Draft
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

pranav-bhatt
Copy link
Contributor

@pranav-bhatt pranav-bhatt commented Oct 28, 2020

#72
Things left to do:

#[] // no_std
pub use String as Url;
#[] // std
pub use url::Url;

@elpiel
Copy link

elpiel commented Oct 28, 2020

For what I've checked, snafu has it's own trait Error that it's implemented in no_std. Since thiserror doesn't have no_std support I would say to just leave snafu for now (if it ain't broken, don't fix it).

@pranav-bhatt
Copy link
Contributor Author

pranav-bhatt commented Oct 28, 2020

For what I've checked, snafu has it's own trait Error that it's implemented in no_std. Since thiserror doesn't have no_std support I would say to just leave snafu for now (if it ain't broken, don't fix it).

Mhmm! The main reason behind wanting to explore thiserror was that it was simpler compared to snafu. If you've worked with any other error handling library that can complement no_std and do the trick, it would be amazing if you could help :)
Would you mind joining the Slack channel so we can continue talking on the #cloudevents-sdk-rust channel? (I promise this is the last switch xD)

@slinkydeveloper
Copy link
Member

slinkydeveloper commented Oct 29, 2020

@pranav-bhatt you need to configure the ci to run the no_std builds/tests too

@@ -0,0 +1,208 @@
//! https://docs.rs/not-io/0.1.0-alpha/not_io/
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This basically is a work around for std::io::Error and std::io::Result<>. If you're asking about the link, then I forgot to remove it :P

Copy link
Member

@slinkydeveloper slinkydeveloper Oct 29, 2020

Choose a reason for hiding this comment

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

Well, why we need io::Error? Can you just disable it in no_std envs?

@@ -1,4 +1,5 @@
use chrono::{DateTime, Utc};
use std::prelude::v1::*;
Copy link
Member

Choose a reason for hiding this comment

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

Can we enable these imports around the sdk only if the no_std feature is enabled?

Copy link
Member

@slinkydeveloper slinkydeveloper left a comment

Choose a reason for hiding this comment

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

In the Cargo.toml, you need to define the std feature, like in https://github.com/KodrAus/rust-no-std/blob/master/Cargo.toml and https://github.com/shepmaster/snafu/blob/master/Cargo.toml and enable it by default.

Then try to create an embedded project in your machine and try to import this library

Cargo.toml Outdated
@@ -25,6 +25,17 @@ base64 = "^0.12"
url = { version = "^2.1", features = ["serde"] }
snafu = "^0.6"

#[dependencies.snafu]
Copy link
Member

Choose a reason for hiding this comment

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

In case of no_std env, you need to import snafu like below (no need to include guide feature)

Other {
source: Box<dyn std::error::Error + Send + Sync>,
},
IOError { source: super::no_std_io::Error },
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need this variant (and the one below) in no_std envs

@slinkydeveloper
Copy link
Member

Can you rebase?

@slinkydeveloper
Copy link
Member

@pranav-bhatt you should look at this #107

@slinkydeveloper
Copy link
Member

@pranav-bhatt can you start cleaning up this pr removing history files etc and rebasing with master? This branch diverged quite a bit from master

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