georust / rstar

R*-tree spatial index for the Rust ecosystem
https://docs.rs/rstar
Apache License 2.0
386 stars 67 forks source link

enable compilation for no_std #83

Closed wucke13 closed 2 years ago

wucke13 commented 2 years ago

This is basically a redo of https://github.com/georust/rstar/pull/27 . I'm not sure whether we should have the alloc feature or not. In the best case, we would haven an rstar implementation which can work without alloc - but my understanding of this implementation is rather limited. Currently, one needs to either pick the std or the alloc feature, picking none of them results in compilation failure. Maybe we can resort to heapless::Vec in the future, but this for sure requires further modifications. This does not include any new features. Compilation is supported for both std and no_std, but the latter requires alloc. For validation, I did this:

wucke13 commented 2 years ago

@Stoeoef regarding checks of the no_std compatibility, maybe we should use cargo-nono, it seems to be made precisely for this use case.

Stoeoef commented 2 years ago

Thanks for the updates!

However, I would still think that a mention in lib.rs and README.md (the one in rstar) would be appropriate. Some short sentence like

# no-std support
rstar supports `no-std` environments by default but requires the `alloc` crate.
wucke13 commented 2 years ago

I added a notice to the list of features (at least to me, no_std compatibiliy is a feaure :). A further note: When I run cargo fmt, even lines which I did not touch are changed. Maybe cargo fmt --checkwould also make sense in the CI?

Stoeoef commented 2 years ago

@Stoeoef regarding checks of the no_std compatibility, maybe we should use cargo-nono, it seems to be made precisely for this use case.

I've checked cargo-nono . Unfortunately, it doesn't produce reliable results for rstar - it marks some of its dependencies as no-std even though they are fine (e.g. smallvec).

Thus, I think an additional check in the CI configuration would be more appropriate. That should consistently catch any conflicting no-std dependency.

Please let me know if I can help you out with anything CI related and I'll see what I can do!

Stoeoef commented 2 years ago

I added a notice to the list of features (at least to me, no_std compatibiliy is a feaure :). A further note: When I run cargo fmt, even lines which I did not touch are changed. Maybe cargo fmt --checkwould also make sense in the CI?

Thanks for adding the entry to the changelog, I agree that this should be included as a feature. In regards to cargo fmt: Captain hindsight agrees with you. However, let's keep this PR focussed for now. Feel free to revert / keep any formatting changes due to cargo fmt as you like.

wucke13 commented 2 years ago

Please let me know if I can help you out with anything CI related and I'll see what I can do!

Please have a look at the workflow file, I believe that this could work but I'm not sure about my usage of env

Stoeoef commented 2 years ago

Please let me know if I can help you out with anything CI related and I'll see what I can do!

Please have a look at the workflow file, I believe that this could work but I'm not sure about my usage of env

Let's give it a

bors try

bors[bot] commented 2 years ago

try

Build succeeded:

wucke13 commented 2 years ago

Lets try that again :)

wucke13 commented 2 years ago

bors try

bors[bot] commented 2 years ago

:lock: Permission denied

Existing reviewers: click here to make wucke13 a reviewer

Stoeoef commented 2 years ago

I'm a little concerned that bors will think everything has passed, even though the std step failed. I'll investigate this while retrying

bors try

wucke13 commented 2 years ago

That was my error, I forgot to add the new job to the needs section of bors' ci-result job despite the angry ALL CAPSLOCK WARNING WHICH TOLD ME TO DO SO.

bors[bot] commented 2 years ago

try

Build succeeded:

Stoeoef commented 2 years ago

bors try

wucke13 commented 2 years ago

I think we have it. I made another small adjustment in the naming of the ci job. If you have nothing left to do, I guess we are good to go?

bors[bot] commented 2 years ago

try

Build succeeded:

Stoeoef commented 2 years ago

Yes! I'm happy as a clam.

Thanks for your work on this PR!

bors r+

bors[bot] commented 2 years ago

Build succeeded: