`no_std` support for `geo-types` #936

wetheredge · 2022-11-19T01:25:06Z

I agree to follow the project's code of conduct.
I added an entry to CHANGES.md if knowledge of this change could be valuable to users.

This is my take on no_std support. This adds a new std that behave similarly to serde's.

Rough overview of my changes:

For the most part, simply swapping alloc or core in for std.
Also added a second doc(hidden) public module (_alloc) to give macros access to Vec and Box even if the crate they are used from does not have extern crate alloc. This could be merged into the existing private_utils. It could also be dropped entirely, but that would require modifying the macros to reference std when that feature is enabled and the cleanest way I found to do that is still somewhat ugly: match on () with cfg'd branches.

I've looked some at #426 and the main difference I see is that it adds core-error for the Error trait, while I've just put that behind the std feature. If that is preferred, I can add the dependency.

I have also started working on no_std support in geo, but it's not quite as straight-forward, so I was keeping it for a second PR.

fixes #422, partially supersedes #426

michaelkirk · 2022-11-23T19:02:09Z

Hello! Thanks for taking a pass at this.

Firstly, I'm not sure if we should be trying to support no_std (that is, maybe we should, I'm just not sure.)

The std->core stuff is easy enough and I don't personally think it's controversial.

But I'm a little hesitant about the alloc feature. It seems like trying to support a no-alloc build is a bit disruptive, and also, to me, it seems unlikely to be useful. Do you actually have this use case? Can I ask what you're using it for?

What do people think about going the route that rstar went, which is no_std but requiring alloc outright?

Also, I think to have any hope of not breaking this in the future we'd need to be sure the no_std build is added to CI.

frewsxcv · 2022-11-23T19:19:16Z

Agreed we should start out by having alloc be required, since it seems to disrupt the code quite a bit, unless there's a really compelling use-case. I think no_std for geo_types is reasonable, but geo might warrant a discussion since that would likely be difficult.

urschrei · 2022-11-23T19:48:01Z

Agreed we should start out by having alloc be required, since it seems to disrupt the code quite a bit, unless there's a really compelling use-case. I think no_std for geo_types is reasonable, but geo might warrant a discussion since that would likely be difficult.

Agreed w/both.

wetheredge · 2022-11-23T22:06:53Z

I added the alloc feature because since that was mentioned as a possibility in the last PR (#426 (comment)) (and the issue #422 (comment)). I'm fine with requiring alloc for no_std. The crate I need this for can't go no-alloc anyway.

I'll go ahead and remove the alloc feature, unless you want to leave it in unused (and documented as such) for the possibility of adding support for no-alloc in a non-breaking release in the future.

Also, I think to have any hope of not breaking this in the future we'd need to be sure the no_std build is added to CI.

I'll add a build on a bare metal ARM target that can't provide std to verify that it works, based on advice from this article.

My other question is whether to add core-error as a dependency so that no_std has an Error impl, since core doesn't yet provide it natively. The last PR did, but I don't know if it's worth another dependency.

geo might warrant a discussion since that would likely be difficult.

Agreed, it's not as simple. I'll take another look at it, perhaps after this is complete, and open a PR for that discussion only if it's not too bad.

michaelkirk · 2022-11-23T22:31:18Z

My other question is whether to add core-error as a dependency so that no_std has an Error impl, since core doesn't yet provide it natively. The last PR did, but I don't know if it's worth another dependency.

I've done very little with no_std. What is common across the no_std ecosystem? What's the cost? What's the benefit?

wetheredge · 2022-11-23T23:36:23Z

What's the cost?

Longer compile time (likely minimal tbf)
Another source for bugs
Looks to be somewhat dead

What's the benefit?

Removes the difference between the std api & the no_std+alloc api

I don't have all that much experience with no_std either, but based on lib.rs/crates.io & GitHub, it looks like the only major crate that uses it is rhai, and only as an optional dependency. Could add it as an optional dep, but that adds yet another feature…. I lean toward not including it at all.

frewsxcv · 2023-01-19T04:17:42Z

@wetheredge Are you still working on this? I think the next step is to require alloc for no_std, instead of making it optional

wetheredge · 2023-01-19T06:27:17Z

Looks like the current CI does run with --no-default-features, but that isn't 100% accurate for testing no_std compatibility: https://blog.dbrgn.ch/2019/12/24/testing-for-no-std-compatibility/. Should I add one of those workarounds? I'm using the build for ARM method in my no_std crate.

frewsxcv · 2023-01-19T12:34:18Z

I think it makes sense to add a --no-default-features build to our CI, yeah

frewsxcv · 2023-01-19T16:14:30Z

Oh disregard my last message, I misread yours. Let me think about this

michaelkirk · 2023-01-19T18:19:18Z

https://github.com/hobofan/cargo-nono looks nice. It hasn't been updated very recently, but maybe that's a sign of maturity vs. abandonment.

frewsxcv · 2023-01-19T19:31:14Z

My vote would be to try adding a new no-std target (e.g. thumbv6m-none-eabi from the blog post) since that simulates the closest thing to what our goal is with no-std. But if that's not feasible then fallback to cargo-nono.

wetheredge · 2023-01-20T03:18:12Z

I've used thumbv7em-none-eabihf since that's what I've got installed and what's in my CI, but I can change it to thumbv6m-none-eabi to match the blog post if wanted. Any of the Tier 2 targets with an asterisk in the middle column should do.

Most of the features are no_std compatible, except for rstar v0.8 & arbitrary. Looks like there's an open PR to add no_std support to arbitrary. Could wait for that, or just document it as incompatible with no_std until that's merged and updated. no_std in rstar v0.8 would likely be pretty simple, tho I haven't checked. rstar v0.9 does work.

frewsxcv · 2023-01-20T03:25:36Z

Let's disable rstar and arbitrary support for now, and if it's something you think you'll need, let's open a follow-up GitHub issue tracking those. No need for this PR to get held up by those dependencies. How does that sound?

wetheredge · 2023-01-20T04:06:32Z

Unless I've missed something, this should be ready.

frewsxcv

lgtm! thank you! will leave it open for another day or two to see if anyone else has feedback

michaelkirk · 2023-01-20T17:04:47Z

This LGTM, thanks!

Is this a breaking change that would necessitate bumping semver? In particular, I'm thinking that introducing a default feature and disabling default features for some of our deps might break things for people.

wetheredge · 2023-01-20T19:53:04Z

Re semver:

The cargo docs aren't entirely clear on adding a new default feature that controls existing functionality. I'd be inclined to say it's technically a major change, but since there weren't any default features before, perhaps you could get away with treating it as minor.

Following the same guidelines, disabling features on dependencies only needs to be a minor change since, as RFC 1105 says, any crate relying on those features without enabling them themselves is a bug.

wetheredge · 2023-02-01T00:15:44Z

Is this ready to merge?

frewsxcv · 2023-02-02T04:37:37Z

Thanks for your patience!

bors r+

936: `no_std` support for `geo-types` r=frewsxcv a=wetheredge - [x] I agree to follow the project's [code of conduct](https://github.com/georust/geo/blob/main/CODE_OF_CONDUCT.md). - [x] I added an entry to `CHANGES.md` if knowledge of this change could be valuable to users. --- This is my take on `no_std` support. This adds a new `std` that behave similarly to `serde`'s. Rough overview of my changes: - For the most part, simply swapping `alloc` or `core` in for `std`. - Also added a second `doc(hidden)` public module (`_alloc`) to give macros access to `Vec` and `Box` even if the crate they are used from does not have `extern crate alloc`. This could be merged into the existing `private_utils`. It could also be dropped entirely, but that would require modifying the macros to reference `std` when that feature is enabled and the cleanest way I found to do that is still somewhat ugly: match on `()` with `cfg`'d branches. I've looked some at #426 and the main difference I see is that it adds `core-error` for the `Error` trait, while I've just put that behind the `std` feature. If that is preferred, I can add the dependency. --- I have also started working on `no_std` support in `geo`, but it's not quite as straight-forward, so I was keeping it for a second PR. fixes #422, partially supersedes #426 Co-authored-by: William Etheredge <[email protected]>

bors · 2023-02-02T04:42:53Z

Build failed:

ci result

geo-types/src/lib.rs

frewsxcv · 2023-02-02T05:08:58Z

It looks like byteorder can't be built on our new target? 🤔

wetheredge · 2023-02-02T05:47:51Z

Oops was installing thumbv6 but building with v7…. Looks like it's fixed.

frewsxcv · 2023-02-02T05:55:22Z

bors r+

bors · 2023-02-02T06:00:43Z

Build succeeded:

ci result

michaelkirk · 2023-02-02T06:41:20Z

Thanks for seeing this through @wetheredge!

wetheredge force-pushed the no_std branch from 914639a to 6893015 Compare January 19, 2023 06:03

no_std support for geo-types

51a62e8

wetheredge force-pushed the no_std branch from 6893015 to 51a62e8 Compare January 19, 2023 06:14

wetheredge added 2 commits January 19, 2023 21:16

Fix features for no_std

a4054eb

no_std cargo check in CI

8b9bedd

wetheredge added 2 commits January 19, 2023 21:57

Update features section of crate docs

bc150a2

Update CHANGES.md

9ce39e1

frewsxcv approved these changes Jan 20, 2023

View reviewed changes

frewsxcv reviewed Feb 2, 2023

View reviewed changes

geo-types/src/lib.rs Outdated Show resolved Hide resolved

wetheredge added 2 commits February 1, 2023 23:35

Improve wording around std requirements

5f0bd98

Install the correct no_std target in CI

758f438

bors bot merged commit d3b07ee into georust:main Feb 2, 2023

wetheredge deleted the no_std branch February 2, 2023 23:54

michaelkirk mentioned this pull request Jun 26, 2023

geo-types: Weaken serde/std and approx/std features #1024

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`no_std` support for `geo-types` #936

`no_std` support for `geo-types` #936

wetheredge commented Nov 19, 2022 •

edited

Loading

michaelkirk commented Nov 23, 2022 •

edited

Loading

frewsxcv commented Nov 23, 2022

urschrei commented Nov 23, 2022

wetheredge commented Nov 23, 2022 •

edited

Loading

michaelkirk commented Nov 23, 2022

wetheredge commented Nov 23, 2022

frewsxcv commented Jan 19, 2023

wetheredge commented Jan 19, 2023

frewsxcv commented Jan 19, 2023

frewsxcv commented Jan 19, 2023

michaelkirk commented Jan 19, 2023

frewsxcv commented Jan 19, 2023

wetheredge commented Jan 20, 2023

frewsxcv commented Jan 20, 2023

wetheredge commented Jan 20, 2023

frewsxcv left a comment

michaelkirk commented Jan 20, 2023 •

edited

Loading

wetheredge commented Jan 20, 2023

wetheredge commented Feb 1, 2023

frewsxcv commented Feb 2, 2023

bors bot commented Feb 2, 2023

frewsxcv commented Feb 2, 2023

wetheredge commented Feb 2, 2023

frewsxcv commented Feb 2, 2023

bors bot commented Feb 2, 2023

michaelkirk commented Feb 2, 2023

no_std support for geo-types #936

no_std support for geo-types #936

Conversation

wetheredge commented Nov 19, 2022 • edited Loading

michaelkirk commented Nov 23, 2022 • edited Loading

frewsxcv commented Nov 23, 2022

urschrei commented Nov 23, 2022

wetheredge commented Nov 23, 2022 • edited Loading

michaelkirk commented Nov 23, 2022

wetheredge commented Nov 23, 2022

frewsxcv commented Jan 19, 2023

wetheredge commented Jan 19, 2023

frewsxcv commented Jan 19, 2023

frewsxcv commented Jan 19, 2023

michaelkirk commented Jan 19, 2023

frewsxcv commented Jan 19, 2023

wetheredge commented Jan 20, 2023

frewsxcv commented Jan 20, 2023

wetheredge commented Jan 20, 2023

frewsxcv left a comment

Choose a reason for hiding this comment

michaelkirk commented Jan 20, 2023 • edited Loading

wetheredge commented Jan 20, 2023

wetheredge commented Feb 1, 2023

frewsxcv commented Feb 2, 2023

bors bot commented Feb 2, 2023

frewsxcv commented Feb 2, 2023

wetheredge commented Feb 2, 2023

frewsxcv commented Feb 2, 2023

bors bot commented Feb 2, 2023

michaelkirk commented Feb 2, 2023

`no_std` support for `geo-types` #936

`no_std` support for `geo-types` #936

wetheredge commented Nov 19, 2022 •

edited

Loading

michaelkirk commented Nov 23, 2022 •

edited

Loading

wetheredge commented Nov 23, 2022 •

edited

Loading

michaelkirk commented Jan 20, 2023 •

edited

Loading