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

Add no_std support for lyon_tessellation #887

Merged
merged 6 commits into from
Dec 17, 2023
Merged

Add no_std support for lyon_tessellation #887

merged 6 commits into from
Dec 17, 2023

Conversation

konall
Copy link
Contributor

@konall konall commented Dec 10, 2023

  • Removed lyon_tessellation rigid dependency on default features from lyon_path
  • Added new crate feature std, enabled by default, which enables std feature of lyon_path dependency
  • Replaced imports from std with their corresponding imports from alloc and core
  • Required enabling feature std to display printed debug assertions
  • Annotated some test functions with #[cfg(test)] to be explicit about their dependence on std

@konall konall marked this pull request as draft December 10, 2023 17:46
@konall
Copy link
Contributor Author

konall commented Dec 10, 2023

I've noticed through testing that the final remaining blocker on no_std support is the dependency on thiserror, which currently does not support it.

A possible alternative would be to use a different error handling (eg: Snafu) that is behind an optional no_std flag so as to be a non-breaking change (it would have to be behind a feature flag because it is currently not possible to exclude dependencies based on feature flags, only to include them: rust-lang/cargo#1839)

I haven't spent enough time looking lyon_tessellation infrastructure to know if swapping out for manually-implemented Errors is a possibility, but that may also work?

I'd be very interested in making this crate no_std compatible so let me know if any of the above are an option you'd be willing to consider.

@konall
Copy link
Contributor Author

konall commented Dec 11, 2023

Having read a bit more into this, I've successfully gotten lyon_tessellation to compile for no_std without any breaking changes (as far as I can tell).

The two big changes were:

  • hand-writing the error implementations that thiserror provided, but feature-gated to std- as asserted here, this is not a breaking change, and I hope that the new error module is concise enough that it doesn't introduce any significant maintenance burden
  • adding a dependency on num_traits for no_std floating point and trig operations- as mentioned above, this dependency unfortunately can't be disabled for std targets, but I copied the exact same way this is set up in the lyon_path crate so I hope it's ok

I'd love to see this merged, so please let me know if there's anything that needs fixing, thanks!

@konall konall marked this pull request as ready for review December 11, 2023 11:40
@nical
Copy link
Owner

nical commented Dec 14, 2023

Hi, thanks for this PR, I haven't had time to look at it yet but I hope to get to it soon.

@nical
Copy link
Owner

nical commented Dec 17, 2023

Looks good, thanks!

@nical nical merged commit 001d713 into nical:master Dec 17, 2023
3 checks passed
@konall
Copy link
Contributor Author

konall commented Dec 17, 2023

Thanks! You may have noticed but with this change the entire lyon crate is now no_std compatible (ignoring the lyon_extra crate of course)- it should be trivial to update lyon to reflect this but let me know if you want a PR submitted

@nical
Copy link
Owner

nical commented Dec 17, 2023

I hadn't noticed! I'm planning to add a fair amount of stuff in lyon's meta crate so let's not mark it as no_std compatible now since I don't know that it can remain that way later. For lyon_algorithm, you are welcome to make the change if you want to, but it's also fine to wait and see if anyone needs it (and submits a PR for it).

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