Skip to content
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

fix: fix CoW in Windows #2

Merged
merged 6 commits into from
Nov 3, 2023
Merged

fix: fix CoW in Windows #2

merged 6 commits into from
Nov 3, 2023

Conversation

nachoaldamav
Copy link
Collaborator

@nachoaldamav nachoaldamav commented Nov 2, 2023

Closes pnpm/pnpm#7186


This PR changes the package to create the reflinks from reflink-copy to copy_on_write, on Unix it is still using reflink-copy underneath.

In the copy_on_write package I have fixed a bug that occurred in ReFS file systems where if you clone the same file to different destinations at the same time (multi-threading basically), the source was modified and filled with null bytes.

The solution to this error is to open the source with the FILE_FLAG_NO_BUFFERING flag (microsoft/CopyOnWrite#12 (comment)).

Also, one important thing, in https://github.com/nachoaldamav/CopyOnWrite I created some Github Actions to run the tests in the specific File systems we want to test, using Google Cloud VMs.

src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
@KSXGitHub
Copy link
Contributor

@nachoaldamav Since you are the author of copy_on_write, can you make it accept impl AsRef<Path> (instead of &str) as function parameters?

src/lib.rs Outdated Show resolved Hide resolved
@nachoaldamav
Copy link
Collaborator Author

@nachoaldamav Since you are the author of copy_on_write, can you make it accept impl AsRef<Path> (instead of &str) as function parameters?

@KSXGitHub Yes, I'll change it

@KSXGitHub
Copy link
Contributor

KSXGitHub commented Nov 2, 2023

I have also noticed that copy_on_write uses EUPL-1.2 license. Is this a strong license or a weak license? Is it compatible with MIT?

@nachoaldamav
Copy link
Collaborator Author

I have also noticed that copy_on_write uses EUPL-1.2 license. Is this a strong license or a weak license? Is it compatible with MIT?

It's permissive, but I'm changing it to MIT, to avoid any issue

Co-authored-by: Khải <hvksmr1996@gmail.com>
@KSXGitHub
Copy link
Contributor

CI failed. Probably because of reference error caused by my suggestions failing to cover all necessary changes.

@nachoaldamav
Copy link
Collaborator Author

CI failed. Probably because of reference error caused by my suggestions failing to cover all necessary changes.

Yes, I'm working on the other side

@nachoaldamav
Copy link
Collaborator Author

@KSXGitHub is this correct? I don't know if the .clone() may be wrong

// Sync version
#[napi(js_name = "reflinkFileSync")]
pub fn reflink_sync(env: Env, src: String, dst: String) -> Result<JsNumber> {
    let src_path = src.clone();
    let dst_path = dst.clone();
    match reflink_file_sync(src_path, dst_path) {
        Ok(_) => Ok(env.create_int32(0)?),
        Err(err) => Err(Error::from_reason(format!(
            "{}, reflink '{}' -> '{}'",
            err.to_string(),
            src,
            dst
        ))),
    }
}

@KSXGitHub
Copy link
Contributor

@nachoaldamav Why didn't you pass it by reference?

let src_path = PathBuf::from(src);
let dst_path = PathBuf::from(dst);
AsyncTask::new(AsyncReflink { src: src_path, dst: dst_path })
AsyncTask::new(AsyncReflink { src, dst })
}

// Sync version
#[napi(js_name = "reflinkFileSync")]
pub fn reflink_sync(env: Env, src: String, dst: String) -> Result<JsNumber> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not familiar with napi, can reflink_sync here can take &str as type for src and dst?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it has to be String, maybe I'm missing something though https://napi.rs/docs/concepts/values.en#string

@@ -46,15 +37,13 @@ impl Task for AsyncReflink {
// Async version
#[napi(js_name = "reflinkFile")]
pub fn reflink_task(src: String, dst: String) -> AsyncTask<AsyncReflink> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This too. Can it take &str as parameter types?

@zkochan zkochan merged commit 808dbb5 into main Nov 3, 2023
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pnpm 8.9.0 broken for windows dev drivers (ReFS)
3 participants