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 no_std build failure #138

Merged
merged 1 commit into from
Aug 10, 2019
Merged

Fix no_std build failure #138

merged 1 commit into from
Aug 10, 2019

Conversation

CryZe
Copy link
Contributor

@CryZe CryZe commented Aug 9, 2019

{f32|f64}::round() does not exist with just core, so libm's implementation is used instead when the std feature is not activated.

@CryZe
Copy link
Contributor Author

CryZe commented Aug 9, 2019

I have no idea what's going on here tbh. cargo test --no-default-features with

#[cfg(not(test))]
use libm::{F32Ext, F64Ext};

spits out

warning: unused import: `F32Ext`
   --> palette/src/lib.rs:172:12
    |
172 | use libm::{F32Ext, F64Ext};
    |            ^^^^^^
    |
    = note: #[warn(unused_imports)] on by default

warning: unused import: `F64Ext`
   --> palette/src/lib.rs:172:20
    |
172 | use libm::{F32Ext, F64Ext};
    |                    ^^^^^^

@Ogeon
Copy link
Owner

Ogeon commented Aug 9, 2019

Hmm... strange, but good catch! I wonder how it passed the tests in the first place. Did you compile with any specific feature set, or just all of them disabled?

Either way, the float trait business is a bit weird and convoluted, but take a look in the float.rs file. That's where we have all the mappings to libm, and that's where you can add this code too.

@CryZe
Copy link
Contributor Author

CryZe commented Aug 9, 2019

Because the tests are always built with std. You can't run tests with no_std. The only way to appropriately check for no_std compatibility is to cargo build --target thumbv6m-none-eabi --no-default-features. (Although what is tested I guess is whether the libm crate works as advertised)

@Ogeon
Copy link
Owner

Ogeon commented Aug 9, 2019

Oh, right! I forgot about that. 🤦‍♂️ Classic and not so strange after all. Do you think you can add a compile test in the test_features.sh file? Both to check this and future mistakes.

@CryZe
Copy link
Contributor Author

CryZe commented Aug 9, 2019

Unfortunately this gets even harder because the lock file also contains the dev dependencies which make the command I posted also not work due to the cargo bug where features of dev dependencies leak into your normal dependencies. So we would need a separate crate specifically to test no_std compatibility that depends on palette. The state of no_std in Rust is honestly still pretty sad tbh.

@Ogeon
Copy link
Owner

Ogeon commented Aug 9, 2019

That's annoying. One would think the dev dependencies would be isolated. In that case it may be better if it's added in a separate issue and PR, to keep the scope of this one focused. I have obviously not investigated the general no_std situation enough, but I suppose it would be good to try to test for it not being leaked, too. So it's doesn't sound like a quick addition.

{f32|f64}::round() does not exist with just core, so libm's
implementation is used instead when the `std` feature is not activated.
Copy link
Owner

@Ogeon Ogeon left a comment

Choose a reason for hiding this comment

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

Great! There's just that one import that doesn't need to be in lib.rs. 🙂

@CryZe
Copy link
Contributor Author

CryZe commented Aug 10, 2019

Too late, that's already gone :P

@Ogeon
Copy link
Owner

Ogeon commented Aug 10, 2019

You are too fast for me 😁

I guess that's why it seemed to glitch a bit when I started the review.

@Ogeon
Copy link
Owner

Ogeon commented Aug 10, 2019

The builds look good so far, so I'll approve this. I'll also create an issue for setting up no_std tests.

@Ogeon
Copy link
Owner

Ogeon commented Aug 10, 2019

bors r+

bors bot added a commit that referenced this pull request Aug 10, 2019
138: Fix no_std build failure r=Ogeon a=CryZe

{f32|f64}::round() does not exist with just core, so libm's implementation is used instead when the `std` feature is not activated.

Co-authored-by: Christopher Serr <christopher.serr@gmail.com>
@bors
Copy link
Contributor

bors bot commented Aug 10, 2019

Build succeeded

@bors bors bot merged commit f913c35 into Ogeon:master Aug 10, 2019
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