Oxidation #656
Closed
Oxidation #656
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Depends on #655 |
jlebon
reviewed
Feb 2, 2017
Makefile.am
Outdated
| endif | ||
|
|
||
| check-local: | ||
| cd $(srcdir)/rust && CARGO_TARGET_DIR=@abs_top_builddir@/target cargo test |
There was a problem hiding this comment.
Do we actually need this target? Or are we planning to use the rust testing framework as well? Might want to rename it to rust-check or similar.
There was a problem hiding this comment.
I think we'd definitely use the Rust unit testing framework, it's really nice. In this case I explicitly chose not to because the idea is we could reuse the exact same tests that were done for the C implementation.
This is an initial drop of "oxidation", or adding implementation of components in Rust. The bupsplit code is a good target - no dependencies, just computation. Translation into Rust had a few twists - - The C code relies a lot on overflowing unsigned ints, and also on the C promotion rules for e.g. `uint8_t -> int32_t` - There were some odd loops that I introduced bugs in while translating...in particular, the function always returns `len`, but I mistakenly translated to `len+1`, resulting in an OOB read on the C side, which was hard to debug. On the plus side, an off-by-one array indexing in the Rust code paniced nicely. In practice, we'll need a lot more build infrastructure to make this work, such as using `cargo vendor` when producing build artifacts for example. Also, Cargo is yet another thing we need to cache. Where do we go with this? Well, I think we should merge this, it's not a lot of code. We can just have it be an alternative CI target. Should we do a lot more right now? Probably not immediately, but I find the medium/long term prospects pretty exciting!
|
🏄 |
|
OK cool. I won't pretend to understand all of the Rust code itself, but the integration itself looks nice! |
|
☀️ Test successful - status-atomicjenkins |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is an initial drop of "oxidation", or adding implementation
of components in Rust. The bupsplit code is a good target - no
dependencies, just computation.
Translation into Rust had a few twists -
also on the C promotion rules for e.g.
uint8_t -> int32_ttranslating...in particular, the function always returns
len,but I mistakenly translated to
len+1, resulting in an OOBread on the C side, which was hard to debug.
On the plus side, an off-by-one array indexing in the Rust code paniced nicely.
In practice, we'll need a lot more build infrastructure to make this work, such
as using
cargo vendorwhen producing build artifacts for example. Also, Cargois yet another thing we need to cache.
Where do we go with this? Well, I think we should merge this, it's not a lot of
code. We can just have it be an alternative CI target. Should we do a lot more
right now? Probably not immediately, but I find the medium/long term prospects
pretty exciting!