-
Notifications
You must be signed in to change notification settings - Fork 103
Rust Analyzer and clippy without cmake #819
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
Conversation
288593b
to
5844f74
Compare
Wow I think it is working 😍 Is it possible to skip the tempdir dependency? Pulling in 3k+ files int the vendor lib is bit heavy, maybe there is a simple alternative that does not need that (cc @x1ddos ) |
cool! yeah I it seems to come from platform independent random number generation.. :( I'm not really happy about it either. |
5844f74
to
90c03df
Compare
I swapped |
90c03df
to
c60907b
Compare
c60907b
to
b74de23
Compare
src/rust/bitbox02-sys/build.rs
Outdated
let tempdir = tempdir.path().to_str().unwrap(); | ||
let _ = Command::new("cmake").arg(&cmake_dir).current_dir(&tempdir).output().unwrap(); | ||
let _ = Command::new("make").arg("rust-bindgen-includes").current_dir(&tempdir).output().unwrap(); | ||
let mut includes_file = File::open(format!("{}/src/rust-bindgen.flags", tempdir)).unwrap(); |
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 panic here if I go for example to src/rust/bitbox02-rust/src/hww/api/set_device_name.rs
and run cargo test
. Any idea?
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'm not sure exactly what you see, but I see a thing that think is wrong. When you run "cargo test" on a crate it will not compile "test versions" of the dependencies. So basically the tests in "bitbox02-rust" should not depend on a feature called "testing" in the "bitbox02" crate.
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.
One workaround seems to be to use cargo test --all-features
. But the "testing" feature isn't additive, so it doesn't compile.
https://doc.rust-lang.org/cargo/reference/features.html
A consequence of this is that features should be additive. That is, enabling a feature should not disable functionality, and it should usually be safe to enable any combination of features.
Scratch that, I had a brainfart. It didn't work with --all-features. It probably has to link in some C code or something...
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.
Good point, I forgot to enable the testing feature, but as you noticed, it still does not work.
Rust-analyzer seems to be working with this PR though, and also clippy seems to work. Just cargo test
seems to fail at this line. The error is:
thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: Os { code: 2, kind: NotFound, message: "No such file or directory" }', bitbox02-sys/build.rs:19:91
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.
Yeah cargo test is more complicated than clippy and RA since it has to do the final step of linking a working executable together. I'll see how hard it would be to support it
a936fa1
to
cee24a4
Compare
It works with depends on #831 Run like this: |
cee24a4
to
c233336
Compare
The reason this depends on #829 or #831 is that this PR crates a cmake build directory in the rust workspace. When running cargo in the rust workspace it gets configured to only use vendored code. So Either prost-build must be vendored or it must be pre-compiled for the cmake build directory to be able to use it when the cmake build directory is in the target dir of the rust workspace. |
c233336
to
434691d
Compare
rebased on master |
396899c
to
c817c42
Compare
This feature is not defined in the bitbox02-rust crate. If I add
Rust-analyzer also seems to not work anymore in this crate, while it worked in a previous iteration of this PR 🤔 Any idea what is going on? |
c817c42
to
f970b6c
Compare
Can you run this outside of docker? I added the testing feature to the bitbox02-rust crate and now it works for me. |
Works on my machine, but needs some testing on other developers setups.
If you can run
cmake build-build && cd build-build && make rust-bindgen-includes
it should work. (this means you have nanopb and bindgen working on your development machine).