fix: build on windows #684

spector-9 · 2024-09-03T12:54:43Z

Currently foyer fails to compile on windows systems. This fixes the errors by adding some line that are cross platform or by adding crates that provide cross platform functionality.

I have written the necessary rustdoc comments
I have added the necessary unit tests and integration tests
I have passed make all (or make fast instead if the old tests are not modified) in my local environment.

the specific steps in the makefile are not compatible with windows. Ran cargo test though*

close #263

- use fs4 crate to get available space rather than using custom function to make it crossplatform.

- Manually seek and read/write. Rather than using read_at and write_at.

- Scope unused function to unix

spector-9 · 2024-09-03T12:56:10Z

foyer-storage/src/device/direct_file.rs

+                let original = file.stream_position()?;
+                file.seek(std::io::SeekFrom::Start(offset))?;
+                let read = file.read(buf.as_mut())?;
+                file.seek(std::io::SeekFrom::Start(original))?;


We can skip this if you are sure that we don't need to seek the file to beginning because this is trying to emulate read_at behavior.

Hi, @spector-9 . I'm not familar with Windows API. On Linux, positioned read/write will bypass modification of fd, which is lighter and faster.

foyer aims to be a hybrid cache with optimal performance. seek io is not acceptable. (BTW, I think there is concurrent bug without a lock to protect seek and io)

@MrCroxx Hi,
That's why I tried to keep unix and windows separate. On windows there's no read_at so there's no other option than to seek and call synchronous_read under the hood also synchronous_read moves the cursor to whatever happens to be the last position.
To be more ergonomic I can call seek_read rather than manually seeking and then reading. I might also be able to remove the final seek that move the cursor to original position if it's not needed.

Oh, sorry about that. Seems my ios safari browser plugin distribes the github diff and I mistook the add and remove (realky wired...). I'll do a careful review tomorrow.

It revented the color in dark mode. 🤣

MrCroxx · 2024-09-03T12:58:16Z

Hi, @spector-9 . Thank you very much for your contribution! It would be great to have foyer run on Windows. But I cannot get bandwidth to do it. This PR helps a lot!!

I'll review it tomorrow. Thank you again for this PR. 🥰

foyer-storage/src/device/direct_fs.rs

foyer-common/src/fs.rs

foyer-storage/src/device/direct_file.rs

MrCroxx · 2024-09-04T02:58:38Z

the specific steps in the makefile are not compatible with windows. Ran cargo test though*

BTW, what build system is most widely used on Windows? Is powershell script okay to replace Makefile?

Co-authored-by: Croxx <[email protected]>

spector-9 · 2024-09-04T05:43:45Z

the specific steps in the makefile are not compatible with windows. Ran cargo test though*

BTW, what build system is most widely used on Windows? Is powershell script okay to replace Makefile?

Honestly I would suggest since this is a rust project use something like nurfile so script/shell is cross-platform. Atleast that's what I use for my projects. If not then I can take a look at makefile and see what can be done to make it work on windows, in a separate PR.

- Use write_all instead of write to handle amount of bytes written

codecov · 2024-09-04T09:52:51Z

Codecov Report

Attention: Patch coverage is 92.30769% with 1 line in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
foyer-common/src/fs.rs	66.66%	1 Missing ⚠️

Files with missing lines	Coverage Δ
foyer-storage/src/device/direct_file.rs	`85.94% <100.00%> (+0.23%)`	⬆️
foyer-storage/src/device/direct_fs.rs	`88.76% <100.00%> (+0.19%)`	⬆️
foyer-storage/src/large/generic.rs	`88.61% <ø> (ø)`
foyer-common/src/fs.rs	`25.92% <66.66%> (-5.11%)`	⬇️

... and 1 file with indirect coverage changes

MrCroxx

Generally LGTM. Thank you for your contribution again. 🥰

Would you mind my helping you solve the problems above? Besides, I want to setup the CI pipeline on windows.

foyer-storage/src/device/direct_file.rs

spector-9 · 2024-09-04T13:59:32Z

@MrCroxx Let me know if I need to do something else. Thanks for your help.

Signed-off-by: MrCroxx <[email protected]>

MrCroxx · 2024-09-05T04:20:51Z

@MrCroxx Let me know if I need to do something else. Thanks for your help.

Hi @spector-9 , Thank you a lot for the contribution. I'll update this branch to try to setup CI on Windows to see if it works.

I'm sorry that this may bring you some inconvenience. You need to update your local branch if needed.

If this CI passed, this PR will be merged ASAP.

Thank you again. 🥰 Wish you enjoy using and developing foyer ~

Signed-off-by: MrCroxx <[email protected]>

MrCroxx

CI on Windows passed! Thank you, @spector-9 . 🥰

MrCroxx · 2024-09-05T07:59:19Z

https://x.com/CroxxMr/status/1831599811273093490

🥰 @spector-9

spector-9 added 5 commits September 3, 2024 15:44

Fix: Foyer-common for windows

c4bc42f

- use fs4 crate to get available space rather than using custom function to make it crossplatform.

Fix: foyer-storage for windows

6dadfb8

- Manually seek and read/write. Rather than using read_at and write_at.

Fix: foyer-util for windows

ab2b21e

- Scope unused function to unix

Fix: foyer storage test

1b6cd9c

Fix: Use read_at/write_at for unix

c074cf5

spector-9 commented Sep 3, 2024

View reviewed changes

MrCroxx reviewed Sep 4, 2024

View reviewed changes

foyer-storage/src/device/direct_fs.rs Outdated Show resolved Hide resolved

foyer-common/src/fs.rs Outdated Show resolved Hide resolved

foyer-storage/src/device/direct_file.rs Outdated Show resolved Hide resolved

MrCroxx added the feature New feature or request label Sep 4, 2024

MrCroxx added this to the v0.12 milestone Sep 4, 2024

MrCroxx changed the title ~~Fix: Build on windows~~ fix: build on windows Sep 4, 2024

spector-9 force-pushed the windows_fix branch 3 times, most recently from 88accc3 to db5cc49 Compare September 4, 2024 05:26

Fix: use seek_(read/write) from file_ext in windows

1f23bf5

spector-9 force-pushed the windows_fix branch from db5cc49 to 1f23bf5 Compare September 4, 2024 05:40

Fix: Replace result::Result with io::Result

7522e38

Co-authored-by: Croxx <[email protected]>

spector-9 requested a review from MrCroxx September 4, 2024 06:10

spector-9 added 2 commits September 4, 2024 12:07

Chore: cargo fmt

46bea16

Fix: Spelling for privileged in docs

a8bd000

spector-9 force-pushed the windows_fix branch from 70654c4 to a8bd000 Compare September 4, 2024 06:38

Fix: foyer storage test

a75176a

- Use write_all instead of write to handle amount of bytes written

MrCroxx reviewed Sep 4, 2024

View reviewed changes

foyer-storage/src/device/direct_file.rs Outdated Show resolved Hide resolved

foyer-storage/src/device/direct_file.rs Outdated Show resolved Hide resolved

spector-9 force-pushed the windows_fix branch from f287802 to 830ff16 Compare September 4, 2024 13:42

Fix: Import statements

99ac2ee

spector-9 force-pushed the windows_fix branch from 830ff16 to 99ac2ee Compare September 4, 2024 13:46

spector-9 requested a review from MrCroxx September 4, 2024 13:53

chore: try to setup ci on Windows

9b2841b

Signed-off-by: MrCroxx <[email protected]>

MrCroxx added 7 commits September 5, 2024 05:55

refactor: remove seek on windows

7acf56a

Signed-off-by: MrCroxx <[email protected]>

chore: remove nix dep

7096212

Signed-off-by: MrCroxx <[email protected]>

fix: remove cargo sort check on windows ci

e77a025

Signed-off-by: MrCroxx <[email protected]>

fix: try fix windows ci with extra deps

683c1c2

Signed-off-by: MrCroxx <[email protected]>

fix: fix ci

d03423f

Signed-off-by: MrCroxx <[email protected]>

chore: fix udeps ci

e827ec7

Signed-off-by: MrCroxx <[email protected]>

fix: fix udeps on windows

79d697f

Signed-off-by: MrCroxx <[email protected]>

MrCroxx approved these changes Sep 5, 2024

View reviewed changes

MrCroxx merged commit 9581d99 into foyer-rs:main Sep 5, 2024
21 of 22 checks passed

MrCroxx modified the milestones: v0.12, v0.11 Sep 5, 2024

MrCroxx mentioned this pull request Sep 5, 2024

chore: move fs4 deps to foyer-storage #685

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: build on windows #684

fix: build on windows #684

spector-9 commented Sep 3, 2024 •

edited by MrCroxx

Loading

spector-9 Sep 3, 2024

MrCroxx Sep 3, 2024

spector-9 Sep 3, 2024

MrCroxx Sep 3, 2024

MrCroxx Sep 3, 2024

MrCroxx commented Sep 3, 2024

MrCroxx commented Sep 4, 2024

spector-9 commented Sep 4, 2024 •

edited

Loading

codecov bot commented Sep 4, 2024 •

edited

Loading

MrCroxx left a comment

spector-9 commented Sep 4, 2024

MrCroxx commented Sep 5, 2024

MrCroxx left a comment

MrCroxx commented Sep 5, 2024

fix: build on windows #684

fix: build on windows #684

Conversation

spector-9 commented Sep 3, 2024 • edited by MrCroxx Loading

Currently foyer fails to compile on windows systems. This fixes the errors by adding some line that are cross platform or by adding crates that provide cross platform functionality.

spector-9 Sep 3, 2024

Choose a reason for hiding this comment

MrCroxx Sep 3, 2024

Choose a reason for hiding this comment

spector-9 Sep 3, 2024

Choose a reason for hiding this comment

MrCroxx Sep 3, 2024

Choose a reason for hiding this comment

MrCroxx Sep 3, 2024

Choose a reason for hiding this comment

MrCroxx commented Sep 3, 2024

MrCroxx commented Sep 4, 2024

spector-9 commented Sep 4, 2024 • edited Loading

codecov bot commented Sep 4, 2024 • edited Loading

Codecov Report

MrCroxx left a comment

Choose a reason for hiding this comment

spector-9 commented Sep 4, 2024

MrCroxx commented Sep 5, 2024

MrCroxx left a comment

Choose a reason for hiding this comment

MrCroxx commented Sep 5, 2024

spector-9 commented Sep 3, 2024 •

edited by MrCroxx

Loading

spector-9 commented Sep 4, 2024 •

edited

Loading

codecov bot commented Sep 4, 2024 •

edited

Loading