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

basic no_std support #1868

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

Conversation

n-prat
Copy link

@n-prat n-prat commented Mar 1, 2023

Hello,

Following up on #1184 (comment) I am opening a PR to discuss
about potential no_std support.

Copy-pasting and rewriting my comment:

TODO?:

  • it still needs more work: I had to butcher error.rs, but I can probably do better.
    • I could optionally move the whole "error.rs refactor" into a separate PR [e.g. replace custom errors by Snafu which supports no_std]
  • I had to be liberal with e.g. #[cfg(feature = "std")] but I can probably replace most of them by a new e.g. io feature.
  • Right now I just aimed to make the core lib compile b/c that is my use case [I need this crate b/c it is a dependency of imageproc which is the one I use directly] → making each and every feature work under no_std is a different beast I am guessing

I'm open to feedback!

I license past and future contributions under the dual MIT/Apache-2.0 license,
allowing licensees to choose either at their option.

[no ci]
Still some doctests(and tests?) FAIL

`cargo build --no-default-features`

[no ci]
`cargo test --no-default-features`
BUT a lot of `#[cfg(feature = "std")]` etc

[no ci]
- `cargo test --no-default-features --features=alloc` -> OK
- `cargo +nightly-2022-10-22 no-std-check --no-default-features` -> FAIL
- `cargo +nightly-2022-10-22 no-std-check --no-default-features --features=alloc` OK
- `cargo test --no-default-features --features=alloc` OK
@n-prat n-prat changed the title Wip no std basic no_std support Mar 1, 2023
Copy link
Member

@HeroicKatora HeroicKatora left a comment

Choose a reason for hiding this comment

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

Seems actually smaller than expected. Without regards to the fundamental issue with the ImageDecoder trait, it seems almost quite feasible to do in single major version.

Except: Getting rid of/bounding DynamicImage to std is quite a deal breaker for adoption. It would seem that an alloc-but-no_std should be capable of preserving the type, or not?

src/image.rs Outdated
#[cfg(feature = "std")]
type Reader: std::io::Read + 'a;
#[cfg(not(feature = "std"))]
type Reader: 'a;
Copy link
Member

Choose a reason for hiding this comment

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

These constraints are not compatible but features should be additives. In particular, an impl written without the feature should still compile with the feature since features may be activated by unrelated third-party crates and are thus shared. This fundamentally can not be written like it is here.

Unless prove of the opposite, i'd assume that whatever magic is necessary to change the behavior, it must happen within the trait bound while keeping the same trait bound in both cases.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd be OK with just limiting ImageDecoder to std. All the implementations have a constructor that takes a io::Read so there's going to be little use for it in no_std settings

Copy link
Author

Choose a reason for hiding this comment

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

I don't think I have gated ImageDecoder behind std. Technically, only its impl using Reader are?

Copy link
Contributor

@fintelia fintelia Mar 3, 2023

Choose a reason for hiding this comment

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

What I was saying is that ImageDecoder should be gated behind std. Because it isn't useful in a no_std context, not just because some of its associated types / methods require standard library traits.

Copy link
Author

Choose a reason for hiding this comment

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

Ah, ok! Done.

src/error.rs Outdated
Comment on lines 30 to 44
#[derive(Snafu, Debug)]
pub enum ImageError {
/// An error was encountered while decoding.
///
/// This means that the input data did not conform to the specification of some image format,
/// or that no format could be determined, or that it did not match format specific
/// requirements set by the caller.
Decoding(DecodingError),
///
/// An error was encountered while decoding an image.
///
/// This is used as an opaque representation for the [`ImageError::Decoding`] variant. See its
/// documentation for more information.
///
/// [`ImageError::Decoding`]: enum.ImageError.html#variant.Decoding
Decoding { format: ImageFormatHint },
Copy link
Member

Choose a reason for hiding this comment

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

Is all of this diff necessary for no_std? Seems a little weird. In particular the comments are no longer accurate. Enum variants are not opaque. It's definitely preferrable to have opaque types to allow changing the field representation / error messages / internal variants.

Copy link
Author

Choose a reason for hiding this comment

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

Yes I think it is...
Previously error types were custom one using std::error::Error but that can not work in no_std.
The only way to make it work is to use a no_std-compatible lib to handle Error and I only found Snafu.

But I had a feeling it was messy to bundle the error.rs refactor with this PR.
Maybe we can do it in two steps:

  • first PR to refactor error.rs to use Snafu
  • "main" PR for no_std

It is a big diff but not a complicated one b/c it is mainly code removal:

  • no need to impl Error when using Snafu
  • no need to impl fmt::Display when using Snafu

Question: not sure if what I did is a breaking change?

Copy link
Contributor

Choose a reason for hiding this comment

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

These changes to ImageError are a breaking change because the variants of public enums are public. What's not clear to me is whether the changes here are actually necessary. Can Snafu really not derive its trait for enums with tuple variants?

Copy link
Author

Choose a reason for hiding this comment

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

No it can not (AFAIK) cf shepmaster/snafu#199 and the various comments and linked issues.

Alternatively there is error_in_core rust-lang/rust#103765 but that would make this crate nightly-only.

Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if we just gate the impl std::error::Error parts behind std? Would that cause any problems? Because it would be nice to avoid the code churn if possible

Copy link
Author

@n-prat n-prat Mar 3, 2023

Choose a reason for hiding this comment

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

Wouldn't that make all the errors not proper errors in no_std context?

But we can go with your idea and instead[starting by reversing all changes in error.rs]:

  • with std: use std::error::Error, same as before
  • no_std: use core::error::Error
    That would make no_std builds require nightly for now; but that is fine for my use case.

Would that be ok for you?
edit: NOTE: seems to be working fine; using #![cfg_attr(feature = "alloc", feature(error_in_core))] so std build should not be affected by the required nightly

For completeness: there is also the "duplicated code" alternative:

  • move original error.rs -> error/error_compat.rs
  • rename this forks error.rs -> error/error_new.rs
  • in error/mod.rs switch impl based on std vs no_std

@HeroicKatora
Copy link
Member

HeroicKatora commented Mar 1, 2023

One comment I want to add outside the workflow of the PR review: If we need to get rid of a core buffer data structure, rewrite the DecoderTrait, etc. completely. Then I'm pretty open to that in a major version as long as there is a clear plan on how to push such rework forward, demonstration of persistent to complete the rework, and keep the test suite in tact while doing it (in a separate branch). That is, somehow keeping track of features that need to be ported. But a new major version is still better than needing an ecosystem split to a different crate, no?

- bump `color_quant`: feature `alloc` renamed `num-traits` cf image-rs/color_quant#21
- `ImageDecoder`: remove `Reader` when no_std b/c cleaner
@n-prat
Copy link
Author

n-prat commented Mar 2, 2023

Except: Getting rid of/bounding DynamicImage to std is quite a deal breaker for adoption. It would seem that an alloc-but-no_std should be capable of preserving the type, or not?

I will try to make it work under no_std but my initial test shows it is not easy.
Indeed it requires eg decoder_to_vec which in turn requires ImageDecoder::read_image_with_progress which in turn requires type Reader: std::io::Read + 'a
I do agree that it should ideally be refactored b/c it makes no sense for "fn with slice input" to require std::io.
edit: I have tried a first pass re-activating most parts of DynamicImage; the parts related to free_functions are still excluded

But why would it be a dealbreaker?

edit: don't really look at the imageproc fork's code; I need to completely rewrite it

- remove Snafu
- require feature `error_in_core` for `no_std`

[ci skip]
Both `std` and `no_std` build+test OK

- `cargo test --no-default-features --features=default` OK
- `cargo +nightly-2022-10-22 test --no-default-features --features=alloc` OK
- `cargo +nightly-2022-10-22 no-std-check --no-default-features --features=alloc` OK
TODO TEMP? or keep it that way?
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