add features for kurbo's std/libm features

[ratmice]

This tries to enable the case where people want peniko with a non-default kurbo features selected.
It does not enable the --no-default-features CI tests because we inherit from kurbo the following compile_error when --no-default-features is selected alone.

https://github.com/linebender/kurbo/blob/6ea9f53498749703606f6f8d019fe0bc9080ea30/src/lib.rs#L83

I personally don't use this in a no_std or anything, I just wanted to go through the list of default-features given the pending release.

[DJMcNab]

Thanks for making these changes so quickly

Do we also need #![no_std] at the top of src/lib.rs?

[DJMcNab]

Do we instead need to check that we still work with only the libm feature?

[ratmice]

Probably, perhaps another option might be to: cargo hack --exclude-no-default-features --feature-powerset
This could reduce the number amount of stuff in ci.yml, but that requires bringing cargo hack into the ci, any preferences?

Edit: actually --exclude-no-default-features --feature-powerset isn't quite right, since we do want --no-default-features --features libm to be tested. Probably better to just add that regardless of cargo hack, and that could go in some other patch if it is wanted.

[ratmice]

Yeah, probably (as I said I don't really use no_std), but adding that pointed out a few places where we were relying on things in the prelude.

[ratmice]

So, this basically runs into kurbo::common::FloatFuncs being a private trait, and these functions being missing on #![no_std] thoughts on making that pub in kurbo, or duplicating them from peniko?

These started triggering only after I added the #![no_std] 🗡️

[raphlinus]

I'd be open to exporting from kurbo, but I'd also be worried because they're not intended to be complete, just what kurbo needs.

[ratmice]

There is currently one function peniko needs, that kurbo doesn't, that is sin/sinf.
patch for exporting those posted at linebender/kurbo#332

[ratmice]

So, this one is passing CI on top of the kurbo PR which has been approved, so if this one looks okay in theory (minus the hard coded revision), I'll go ahead an land that one, then we can work towards getting rid of that hard coded rev here?

[DJMcNab]

I'm surprised we compile with default features enabled now - https://github.com/linebender/kurbo/blob/6ea9f53498749703606f6f8d019fe0bc9080ea30/src/lib.rs#L80 enables this conditionally

Clearly if CI passes, it does work though? Any explanation

[ratmice]

There are kind of 2 things going on, the cfg_attr there allows #[test] to use std, even though the library is linked no_std, while peniko currently doesn't have any tests.

The other thing that is relevant is this warning at the bottom of the following:
https://doc.rust-lang.org/reference/names/preludes.html#the-no_std-attribute

Using no_std does not prevent the standard library from being linked in. It is still valid to put extern crate std; into the
crate and dependencies can also link it in.

So, my understanding was that std being a re-export of core, etc, when build with default features this just uses those re-exported libs.

I'm definitely no no_std expert though (that was my assumption though) so in this sense, I understand why the code in your link is doing it conditionally on test, but not on the feature = "std".

[ratmice]

So, I removed the features = "std" from kurbo leaving just #![cfg_attr(not(test)), no_std)] the thing that is going on there is the svg module in lib.rs #[cfg(feature = "std")] mod svg and uses std freely rather than core/alloc.

This is part of the reason actually that cargo test --no-default-features --features libm breaks on kurbo :D
Because that should be #[cfg(any(feature = "std", test)))] mod svg; I think.

[ratmice]

Anyhow to summarize my feeling here is that the feature = "std" part of that cfg_attr is specific to kurbo only conditionally exporting the svg module, and this combination becomes pretty fragile. As evidenced by the fact that cargo build --no-default-features --features libm works but cargo test ... compile fails on a few things. So I would rather avoid that aspect.

The test part of the cfg_attr we may want to add now or in the future, but it doesn't currently affect us absent tests.

[DJMcNab]

So, this suggests that to test properly, we need to build Peniko with the libm feature on a platform without the standard library available?
Because at the moment (ignoring Kurbo) the reason that features=std works is because the std float functions are being used, but nothing about the feature itself actually enables those? Is there a cargo command to force non-usage of std on all targets?

[ratmice]

Ahh, now I see the problem you are indicating, that even though the lib defined #![no_std] and never imports std, the kurbo feature enabling causes std to be included thus f32::sin and friends work without error.

I don't see anything that looks solid beyond what you suggest (building for a target that doesn't have std).
There is some discussion and a PR here.
rust-lang/rust-clippy#1570

[waywardmonkeys]

If it helps, that's why kurbo has a CI thing that builds for thumbv7m-none-eabi.

[ratmice]

It does, I had to go with armv7a-none-eabi because of AtomicU64, but tested that that fails with default features,
and succeeds with --no-default-features --libm.

[DJMcNab]

Looks good, pending the kurbo release

[DJMcNab]

I'd probably add this to the step which adds the rust toolchain, similarly to the WASM task

[ratmice]

Should be fixed in 241ddb4

[ratmice]

Thank you for all the reviewing Daniel, I went ahead and merged the kurbo patch, so once that has seen a release will update the Cargo.toml here.