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

no_std support #21

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

n-prat
Copy link

@n-prat n-prat commented Dec 19, 2022

Hello,

I am currently in the process of porting image and imageproc to work in SGX env, and I hit the issue of this crate using std.

So I am opening the PR to see if this could interest you.
And if so, I could clean up the commits, and/or rewrite them a bit more cleanly.

Same principle as nalgebra,simba,etc
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.

The need and no_std seems okay, but I don't quite get the manner in which it was implemented.

We've not been used num-traits before, no line of code is generic on floating point types so what motivates that dependency? Some methods aren't available in no_std, which is okay although you could elaborate and sum up which of them caused errors. Using libm as a supplement for these is entirely reasonable but num-traits is not necessary to achieve this. I'd rather not have it.

src/lib.rs Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
@n-prat
Copy link
Author

n-prat commented Dec 19, 2022

I am definitely not a Rust expert, so there may be a better way to it.

In any case, without that use num_traits we get:

color_quant$ cargo +nightly test
   Compiling color_quant v1.1.0 (/XXX/color_quant)
error[E0599]: no method named `abs` found for type `f64` in the current scope
   --> src/lib.rs:302:30
    |
302 |             dist = (n.b - b).abs();
    |                              ^^^ method not found in `f64`

error[E0599]: no method named `abs` found for type `f64` in the current scope
   --> src/lib.rs:303:31
    |
303 |             dist += (n.r - r).abs();
    |                               ^^^ method not found in `f64`

error[E0599]: no method named `abs` found for type `f64` in the current scope
   --> src/lib.rs:305:35
    |
305 |                 dist += (n.g - g).abs();
    |                                   ^^^ method not found in `f64`

error[E0599]: no method named `abs` found for type `f64` in the current scope
   --> src/lib.rs:306:35
    |
306 |                 dist += (n.a - a).abs();
    |                                   ^^^ method not found in `f64`

error[E0599]: no method named `round` found for type `f64` in the current scope
   --> src/lib.rs:394:58
    |
394 |             self.colormap[i].b = clamp(self.network[i].b.round() as i32);
    |                                                          ^^^^^ method not found in `f64`

error[E0599]: no method named `round` found for type `f64` in the current scope
   --> src/lib.rs:395:58
    |
395 |             self.colormap[i].g = clamp(self.network[i].g.round() as i32);
    |                                                          ^^^^^ method not found in `f64`

error[E0599]: no method named `round` found for type `f64` in the current scope
   --> src/lib.rs:396:58
    |
396 |             self.colormap[i].r = clamp(self.network[i].r.round() as i32);
    |                                                          ^^^^^ method not found in `f64`

error[E0599]: no method named `round` found for type `f64` in the current scope
   --> src/lib.rs:397:58
    |
397 |             self.colormap[i].a = clamp(self.network[i].a.round() as i32);
    |                                                          ^^^^^ method not found in `f64`

For more information about this error, try `rustc --explain E0599`.
error: could not compile `color_quant` due to 8 previous errors
warning: build failed, waiting for other jobs to finish...

@n-prat
Copy link
Author

n-prat commented Dec 19, 2022

I have done a bit of clean up: it now works both for cargo +nightly test --no-default-features --features=alloc and cargo +nightly test.
My bad, I should have done it this way in the first place.

NOTE: I have put num-traits behind a new alloc feature b/c it seems to be the convention, but I can change it if you want?

PS: the #[allow(unused_imports)] is still needed; I really don't get why.
But I quick googling around shows a lot of people doing the same thing.

@HeroicKatora
Copy link
Member

NOTE: I have put num-traits behind a new alloc feature b/c it seems to be the convention, but I can change it if you want?

Isn't it possible to use libm::* directly? The traits are just an indirection to floating point methods from that dependency it seems. The majority of the build costs / dependency overhead are in num-traits due to its auto-cfg build dependency.

@n-prat
Copy link
Author

n-prat commented Jan 11, 2023

Probably not? I am not using the libm feature of num-traits.
Indeed FloatCore does not seem to use libm.

@HeroicKatora
Copy link
Member

Huh, I wasn't aware num-traits was actually implementing some IEEE math itself without libm. My perception was they'd just defer to one of the available methods.

@n-prat
Copy link
Author

n-prat commented Feb 13, 2023

Hi,

I have downgraded the edition to 2018 to make it compatible with Rust 1.34(not 100% sure about this one) [even though I don't really see the point of compiling with a 3 years old compiler?].
And more importantly, I have added both default and alloc feature to the CI matrix.

Also, do you have a better idea for the feature's name because alloc is not really appropriate. Maybe num_traits? Something else?

@HeroicKatora
Copy link
Member

HeroicKatora commented Mar 1, 2023

After some consideration of the API, there's some good ideas for a renovation and 2.0 release anyways. For instance map_pixel and index_of should take arrays respectively. Thus, I'd personally be fine with bumping the minimum-rust-version to 1.56 (or higher if needed). Not to mention that technically the feature selection is breaking if downstream somewhere default-features = false was selected. Eh, little thing.

@HeroicKatora
Copy link
Member

Also, do you have a better idea for the feature's name because alloc is not really appropriate. Maybe num_traits? Something else?

Generally, features should be (super-) additive, not subtractive. In this sense, num_traits is a fallback for a missing other feature (std). There's a possible selection between floating point operations from std and those from num_traits. So, num_traits itself would be appropriate to use as a feature. You can use optional dependencies in cfg(feature = …) to detect it.

@HeroicKatora
Copy link
Member

So, there could be merely two features: std, and num_traits as an optional dependency. Then the crate would require either to work, and ignore num_traits if std is also selected.

- update #[cfg] logic in lib.rs
- ci: add "no-std-check" with "features=num-traits"
@n-prat
Copy link
Author

n-prat commented Mar 2, 2023

Allright.

So I have:

  • renamed "alloc" feature to "num-traits"[I used - instead of _, bot sure about naming convention?]
  • added no-std-check to the CI
  • bumped Rust to 1.36 b/c needed for alloc crate

FIY this PR is blocking for image-rs/image#1868
But not sure how to set things up b/c color_quant is both a direct dep of image[ok b/c we can bump this create version in the linked PR] and an indirect dep of gif...

@n-prat
Copy link
Author

n-prat commented Mar 2, 2023

NOTE: if you would rather keep min Rust to 1.34 I could alternatively gate the use alloc behind #[cfg]?

@HeroicKatora
Copy link
Member

I'd prefer 1.56 and adding [package.rust-version].

n-prat added a commit to Interstellar-Network/image that referenced this pull request Mar 2, 2023
- 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

Done.

I have also removed the if: ${{ matrix.rust in .github/workflows/rust.yml b/c it should not be needed anymore?

Cargo.toml Outdated Show resolved Hide resolved
@n-prat
Copy link
Author

n-prat commented Mar 2, 2023

I have added a no_std to the README. Is that OK? Do you require changes?

@n-prat
Copy link
Author

n-prat commented Mar 2, 2023

I had forgotten to install "cargo-no-std-check". Should hopefully be good now.

@n-prat n-prat changed the title Sgx nostd compat no_std support Mar 2, 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.

2 participants