-
Notifications
You must be signed in to change notification settings - Fork 15
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
Support no_std
targets
#68
Conversation
I don't know how to modify CI to test this. Locally, I ran the following for testing:
|
To test this as part of the CI you'll need to add a custom pipeline. Here is an example of a custom pipeline in another repository: https://github.com/rust-vmm/vm-virtio/tree/main/.buildkite. Once we get close with merging this PR, I can make the pipeline run on all PRs by configuring it in Buildkite. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, I have just some questions. Thanks for handling this!
Cargo.toml
Outdated
@@ -8,7 +8,12 @@ edition = "2018" | |||
repository = "https://github.com/rust-vmm/vm-fdt" | |||
keywords = ["fdt"] | |||
|
|||
[dependencies] | |||
hashbrown = "0.14" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are trying to keep the dependencies to a minimum, and for this crate we have none. Is there a way to have this at least as an optional dependency?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no nice way of doing that since Cargo features are additive and feature = "std"
would only add things.
We could add an alloc
feature that has to be enabled explicitly. That way cargo check --no-default-features
without any features will fail though, and one has to enable alloc
explicitly on no_std
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess the other alternative would be to just skip duplicate phandle checking in the no_std
version so we would not need a HashSet
at all; that would make it the caller's responsibility to ensure phandles are unique (like it was before we added this check). Maybe tying that to no_std
would be too confusing, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think, I'd prefer not have this compile on no_std
without a feature instead of removing functionality from this crate. I'll implement this shortly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you okay with the solution I implemented? :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's fine as it is, I was just wondering if it's possible.
eca10f3
to
f51c6c7
Compare
I added a CI config. 👍 |
I added the custom pipeline in Buildkite and retriggered the build. Any other PRs open against vm-fdt will fail the tests until this one is merged. If this becomes a problem I can also de-activate the custom build. |
This adds `no_std` support by - adding a default `std` feature for disabling `impl Error` on `no_std`. - adding the Hashbrown dependency for `no_std` hash sets Hashbrown also provides the `HashSet` implementation in `std`. `std`'s `HashSet` is not in liballoc yet, though. By default, Hashbrown uses AHash as the default hasher. AHash is much faster than `std`'s SipHash, but is not as HashDoS resistant. That seemed not relevant to me. Co-authored-by: Andreea Florescu <andreea.florescu15@gmail.com> Signed-off-by: Martin Kröning <martin.kroening@eonerc.rwth-aachen.de>
3c2f4c7
Thanks :) Sorry for dismissing the reviews. I force-pushed the fix for the CI. |
"tests": [ | ||
{ | ||
"test_name": "build-alloc", | ||
"command": "rustup target add {target_platform}-unknown-none && env RUSTFLAGS=\"-D warnings\" cargo build --release --target {target_platform}-unknown-none --no-default-features --features alloc", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The rustup target add should actually go in the rust-vmm container. We can merge this as a temporary workaround, but we should eventually fix it. Do you want to send a PR with this change in rust-vmm-container as well? This is where we need to add the target: https://github.com/rust-vmm/rust-vmm-container/blob/main/build_container.sh#L40. If not, I can do that some time later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. I opened rust-vmm/rust-vmm-container#94.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am merging this PR, and once we publish a new version of rust-vmm-ci with the updated rust-vmm-container we can update this as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I opened #70 so we don't forget about this.
Thanks a lot! When can we expect a release? :) |
I can just create one now. |
Summary of the PR
This adds
no_std
support bystd
feature for disablingimpl Error
onno_std
.no_std
hash setsHashbrown also provides the
HashSet
implementation instd
.std
'sHashSet
is not in liballoc yet, though.By default, Hashbrown uses AHash as the default hasher. AHash is much faster than
std
's SipHash, but is not as HashDoS resistant. That seemed not relevant to me, though.Closes #61
Requirements
Before submitting your PR, please make sure you addressed the following
requirements:
git commit -s
), and the commitmessage has max 60 characters for the summary and max 75 characters for each
description line.
test.
Release" section of CHANGELOG.md (if no such section exists, please create one).
unsafe
code is properly documented.