WIP: preparing conditional compilation for multiple os#148
Conversation
bpetit
changed the title
…ll rust environment twice
bpetit
force-pushed
the
feat/conditional-compilation
branch
from
0de123c to
c18855c
Compare
|
Here is the repo of the driver for reading MSRs on windows : https://github.com/hubblo-org/windows-rapl-driver |
uggla
left a comment
uggla
left a comment
There was a problem hiding this comment.
Pretty large MR hard to review. 123 commits is a bit too much. :)
It would have been good to split them into several topics (CI, Win,...).
Anyway, I have reviewed it, hoping I have not missed too much stuff.
Overall it looks ok, and I understand what you did. Good work ! Especially the windows driver part, which looks not trivial ! 👍
I think some functions could be refactored to reduce nested levels. This is particularly true in one function in the windows driver part that is hard to read due to unsafe and if let blocks plus formating.
Outside of this review, I saw a lot of signatures with Result<xxxx, String>.
To my mind, maybe signatures need to be reviewed to Option or error handling could be improved.
That's all for this first pass, but maybe another one would be required as my focus decreased at the end of the review.
| uses: actions-rs/cargo@v1 | ||
| with: | ||
| command : clippy | ||
| args: --no-default-features --features "prometheus json riemann" |
There was a problem hiding this comment.
Why do you restrict to such features and not use the default ones?
There was a problem hiding this comment.
For now, other exporters require openssl crate which asks for linux openssl libraries.
Once we hare removed this requirements for rustls we should be able to allow all features on windows.
Cargo.toml
Outdated
| warp10 = { version = "1.0.0", optional = true } | ||
| rand = { version = "0.7.3" } | ||
| time = "0.2.25" | ||
| lazy_static = "1.4.0" |
There was a problem hiding this comment.
Hum, I was curious to know where it is used.
But it appears to be not used in the code unless I'm wrong. So I think, it could be removed.
uggla feat/conditional-compilation ~ workspace rust scaphandre rg -l lazy_static
Cargo.toml
Cargo.lock
| hostname: String, | ||
| qemu: bool, | ||
| watch_containers: bool, | ||
| _qemu: bool, |
There was a problem hiding this comment.
What is the reason to rename them to _qemu and _watch_containers ?
src/sensors/mod.rs
Outdated
| @@ -158,14 +192,33 @@ impl Topology { | |||
| /// } | |||
| /// ``` | |||
| pub fn generate_cpu_cores() -> Result<Vec<CPUCore>, String> { | |||
There was a problem hiding this comment.
As there is no ? or Err below.
Should the signature be changed to an option and not a Result.
Anyway, the above doc is up2date and needs to be changed.
There was a problem hiding this comment.
changed it in d413af5
Does that make sense ?
src/sensors/mod.rs
Outdated
| .map(IProcess::from_linux_process) | ||
| .collect::<Vec<_>>(); | ||
|
|
||
| info!("Before refresh procs loop."); |
There was a problem hiding this comment.
I think it can be refactored to reduce the number of nested levels and make it easier to read.
There was a problem hiding this comment.
What do you think about this ?
| .values() | ||
| .map(IProcess::from_windows_process) | ||
| .collect::<Vec<_>>(); | ||
| for p in current_procs { |
There was a problem hiding this comment.
I'll do the same as e9c96cb on windows, depending on your appreciation :)
| } | ||
| } | ||
| impl RecordReader for CPUSocket { | ||
| fn read_record(&self) -> Result<Record, Box<dyn Error>> { |
There was a problem hiding this comment.
Fonction really hard to read due to nested blocks and formating.
I think a refactor with functions to flatten it would help.
There was a problem hiding this comment.
It is a bit badly written, as I had a hard time at first to make send_request work properly.
I intend to push windows support in 0.5 as an experimental feature, then work on packaging and improvements for the 0.6.
Do you think it could wait the 0.6 ?
| pub tty_nr: i32, | ||
| pub tpgid: i32, | ||
| pub flags: u32, | ||
| //pub minflt: u64, |
There was a problem hiding this comment.
comments to remove ? Here and below
There was a problem hiding this comment.
I agree it's not that pretty to have all those comments, but this is kind of a reminder to explore what procfs and sysinfo have to offer.
2 participants
No description provided.