build on windows system. #322
Conversation
|
@tabokie PTAL |
There was a problem hiding this comment.
Some CI changes that're needed:
- Add windows to
raft-engine/.github/workflows/rust.yml
Line 13 in 1c7a820
- Makefile/Cargo.toml needs some heavy updates so that the newly introduced "_use_std_fs" is only enabled when running
make test_with_std_fs, and disabled otherwise - Add
make test_with_std_fsto the coverage pipeline:raft-engine/.github/workflows/rust.yml
Line 100 in 1c7a820
|
@tabokie PTAL. Can you approve the workflow to run so I can debug the windows build in CI? |
|
@tabokie can you make this PR always run workflow? Need another approval again. Debugging github workflow is a pain, especially for os = windows. I can see more commits to debug it in the future. |
|
I don't have the permission to do that. It's weird though because my fork repo runs the workflow just fine: https://github.com/tabokie/raft-engine/actions, I wonder why your fork repo ignores the workflow file. |
|
@MichaelScofield You can work around this by merging another small PR first. E.g. Open another PR to update clap manually (#306) |
|
@tabokie |
|
It can build now, you just need to fix the tests. |
|
Running into infamous "access denied (os error 5)" error in windows. Convert this PR to draft first, to reduce github notification noise. |
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #322 +/- ##
=======================================
Coverage 97.98% 97.99%
=======================================
Files 31 33 +2
Lines 12329 12388 +59
=======================================
+ Hits 12081 12140 +59
Misses 248 248
☔ View full report in Codecov by Sentry. |
| use std::path::Path; | ||
| use std::sync::Arc; | ||
|
|
||
| pub struct LogFd(Arc<RwLock<File>>); |
There was a problem hiding this comment.
This implementation kind of defeats the point of Handle abstraction, which is to be efficiently shared between multiple readers/writers. I like seeing a fallback solution that covers all platforms other than Windows, but this implementation is too inefficient for production use.
I can think of two ways to go around this:
(1) use raw windows HANDLE here like in #69, it has the downside of windows-only.
(2) further separate LogFile into unix and windows version. The windows version looks something like below. It has the downside of needing to port lots of failpoints again.
struct LogFd(File);
struct LogFile(File);
impl LogFd {
fn new_writer(&self) -> LogFile {
LogFile(self.0.try_clone().unwrap())
}
}
|
@MichaelScofield #325 is merged. Feel free to ask questions. #322 (comment) might be too much work for this PR, you can follow up in another one or let me handle it later. |
|
@tabokie Sorry for the late reply, busy week. I actually considered using the raw Windows API earlier. However, skimmed through the "windows" crate and the "official win32 API docs", I find it's not a quick and easy task. It requires detailed understanding of Windows filesystem to do it well, which I'm currently lacking of, nor do I have the time to study it. Besides, the windows binding APIs are all "unsafe" to call in rust. So I decide to switch back to the good old std fs, make it right first. I'll make the features worked in the new test matrix this weekend. |
|
@tabokie tests are ok now, PTAL |
|
@tabokie PTAL Now the "test_matrix" is only used in coverage job, and only runs under ubuntu. |
Signed-off-by: luofucong <luofc@foxmail.com>
|
@tabokie PTAL |
|
GitHub action space is not enough.. I'll take a look tomorrow. |
Let cargo build pass under windows system:
fork.rsLogFdimplementations in each os. Guarded (or choosen) by feature flagsstd::fsAPIs insteadnixraw bindings