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

support ics23 no-std #41

Merged
merged 5 commits into from
Jul 12, 2021
Merged

support ics23 no-std #41

merged 5 commits into from
Jul 12, 2021

Conversation

DaviRain-Su
Copy link

Support Ics23 rust no-std features.

@ethanfrey
Copy link
Contributor

Interesting and thank you for the contribution. I will look later.

In the meantime, could you update the CI scripts so they will run tests in no_std mode (--no-defaut-features, correct?) as well as the normal std mode?

@DaviRain-Su
Copy link
Author

Interesting and thank you for the contribution. I will look later.

In the meantime, could you update the CI scripts so they will run tests in no_std mode (--no-defaut-features, correct?) as well as the normal std mode?

Ok ah, I'll see how to set it up.

@ethanfrey
Copy link
Contributor

Thank you.

@ethanfrey
Copy link
Contributor

I just checked out your branch and tried running no_std tests:

cargo test --all --no-default-features

This produces a number of errors. Most are either:

cannot find function decodein cratehex``
or
vec![valid_inner.clone()]

with suggestions for no_std_compat::prelude imports.

@ethanfrey
Copy link
Contributor

Can you add these lines to the end of the circleCI job?

      - run:
          name: Build all targets in no_std
          command: cargo build --all --no-default-features
       - run:
           name: Run all tests with no-std
           command: cargo test --all --no-default-features

@DaviRain-Su
Copy link
Author

cargo test --all --no-default-features

It doesn't seem to work.

@ethanfrey
Copy link
Contributor

cargo test --all --no-default-features

It doesn't seem to work.

Exactly, the tests fail to compile (build works, but test code seems to not be no_std compatible).
Can you try fixing those, at least obvious issues. If something requires deeper changes to the codebase, I can take a look.

@DaviRain-Su
Copy link
Author

cargo test --all --no-default-features

It doesn't seem to work.

Exactly, the tests fail to compile (build works, but test code seems to not be no_std compatible).
Can you try fixing those, at least obvious issues. If something requires deeper changes to the codebase, I can take a look.

I solved this problem, see if you can pass it.

@DaviRain-Su
Copy link
Author

@ethanfrey Hi, Can you merge this PR?

@ethanfrey ethanfrey mentioned this pull request Jul 12, 2021
4 tasks
Copy link
Contributor

@ethanfrey ethanfrey left a comment

Choose a reason for hiding this comment

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

Sorry for the late review. I was on vacation the last two weeks.

Thank you for adding no-std to the CI and getting tests to pass. I created a follow up issue #42 that it would be great if you could look at sometime. But for now, I will merge your changes to unblock you. It would be nice if we could tackle at least some of #42 before I tag and push a new crate

@@ -230,6 +236,7 @@ mod tests {
pub value: Option<Vec<u8>>,
}

#[cfg(feature = "std")]
Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, so these tests are not run in no_std mode.

It gets the code to pass (and I guess I can merge it), but it would be nice to run the whole test suite with no-std

Copy link
Author

Choose a reason for hiding this comment

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

I think it is necessary to test here in the std environment, for File ,io and some other libraries are not available in core and alloc, but only in the standard library.
Relate tourtial: https://docs.rust-embedded.org/book/intro/no-std.html

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah... you are very right.

I guess none of these work in no-std but it would be nice to have equivalent tests.
Maybe you can use include_bytes, and then get rid of all the load_file stuff as well (and have tests work in both environments)

@@ -1,3 +1,11 @@
#![cfg_attr(not(feature = "std"), no_std)]
#![allow(dead_code)]
Copy link
Contributor

Choose a reason for hiding this comment

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

funny smell here.
I guess some code is only exposed via std or no-std?
We could also just conditionally expose it.

Copy link
Author

@DaviRain-Su DaviRain-Su Jul 19, 2021

Choose a reason for hiding this comment

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

Yes, compile by using #![cfg_attr(not(feature = "std"), no_std)] to conditionally select between std and no_std.
Here some tourtial: https://justjjy.com/Rust-no-std

Copy link
Contributor

Choose a reason for hiding this comment

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

I was commenting on #![allow(dead_code)], which I removed in another PR

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.

2 participants