Conversation
libwild/src/timing.rs
Outdated
|
|
||
| const PERFETTO_ENV_VAR: &str = "WILD_PERFETTO_OUT"; | ||
|
|
||
| #[cfg(feature = "perfetto")] |
There was a problem hiding this comment.
I just published version 0.3.0 of perfetto-recorder, which adds support for Windows. At least it builds and the tests pass on Windows.
libwild/src/archive.rs
Outdated
|
|
||
| #[cfg(not(unix))] | ||
| fn path_from_bytes(bytes: &[u8]) -> PathBuf { | ||
| PathBuf::from(String::from_utf8_lossy(bytes).into_owned()) |
There was a problem hiding this comment.
If a path is supplied that isn't a valid Windows path, then from_utf8_lossy won't do what we want. Most likely we'd end up reporting that the file couldn't be found which would be confusing. Reporting an error or even panicking would likely be better.
libwild/src/archive.rs
Outdated
| inner(path, limit).with_context(|| format!("Failed to process {}", path.display())) | ||
| } | ||
|
|
||
| #[cfg(unix)] |
There was a problem hiding this comment.
Rather than using cfg to disable tests, how about using #[cfg_attr(not(unix), ignore)]. That way the test will still be built, it just won't be executed. That will then mean that you won't get dead code warnings for stuff that was only used by that test.
|
|
||
| pub(crate) fn make_executable(file: &File) -> Result { | ||
| #[cfg(unix)] | ||
| use std::os::unix::prelude::PermissionsExt; |
There was a problem hiding this comment.
Can this use-statement just be moved inside the cfg block below?
8Keep
force-pushed
the
windows-build-ci
branch
4 times, most recently
from
e0b2a72 to
04bb282
Compare
|
Thanks for the review. I've pushed a new version. Some notes:
Cargo build+test now pass |
|
Thanks. Looks good. A few CI failures. Looks like you need to run |
8Keep
force-pushed
the
windows-build-ci
branch
2 times, most recently
from
9dc20fc to
9e58271
Compare
|
Ok, I've fixed some lints/errors/format. I did use cfg a bit more to make the executable bit code only available on unix, let me know if you'd prefer something else there. The test error was due to clrf endings on windows, and git was thinking that .a file is a text file. I've added a .gitattributes so git doesn't treat it like a text file. I have that config set locally, so I didn't see that error locally. I think this would be right... Let's see what ci says now |
|
Looks like there's just one clippy warning left. The clippy command that gets run in CI is |
| #[cfg(unix)] | ||
| { | ||
| let _ = crate::fs::make_executable(&self.file); | ||
| } |
There was a problem hiding this comment.
Maybe pull the #[cfg(unix)] into the make_executable body?
Edit: Didn't notice your comment above.
| ) | ||
| })?; | ||
| } | ||
| #[cfg(not(unix))] |
There was a problem hiding this comment.
I think it would be cleaner to write this as:
if meta.is_dir() {
// ...
return Ok(());
}
#[cfg(unix)]
if meta.is_symlink() {
// ...
return Ok(());
}
if let Ok(data) = FileData::new(source_path, false) {
// ...
}
// hardlink or copy
|
Nothing heard. @bjorn3 would you like to pick this up or should I? |
|
I ran into the tidy::check_sources_format failure locally. It turns out the failure is purely due to clang-format enforcing brace spacing in void foo(void) {};
→ void foo(void){};After running Mentioning this in case anyone else hits the same failure while testing this PR. |
|
So sorry everyone, I've just been very busy and out of town most of the time since. If you need it soon - feel free to take it over |
4 participants
I am maybe/sorta going to try to get wild working on windows. The first (easy step to that is to get it buildable on windows. This PR gets it building on windows, disables some tests that don't yet work, and sets up github actions CI to also build+test on windows.