Skip to content

Add windows os support#48

Open
jonathanmorley wants to merge 2 commits into
iddm:masterfrom
jonathanmorley:windows
Open

Add windows os support#48
jonathanmorley wants to merge 2 commits into
iddm:masterfrom
jonathanmorley:windows

Conversation

@jonathanmorley

Copy link
Copy Markdown

No description provided.

@iddm iddm left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Although this PR heads towards a good goal, it needs some changes before merging.

Comment thread src/deploy.rs

let send_file = |sess: &Session, local_path: &Path, remote_path: &Path| {
let mut buffer = vec![0; *PAGE_SIZE as usize];
let mut buffer = vec![];

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I have used PAGE_SIZE especially because it is good for performance to read memory in size multiple to page size. That is why I used it. Windows has also page size parameter, and almost every OS has it actually. It is also possible to read it in windows. As for the rust language, the function to take this constant is probably is defined either in libc crate or winapi crate, I don't know. What's more important, removing the constant as you did here simply will not work: we must allocate some memory to read into later in the line:

while let Ok(read_bytes) = file.read(&mut buffer) {

As we did not allocate anything in the vector above, our vector size is zero and so we will try to read 0 bytes from the file and it will last forever. You may try doing this in a simple example:

fn main() {
    use std::io::Read;

    let mut f = std::fs::File::open("r.rs").unwrap();
    let mut buf = vec![];

    while let Ok(read_bytes) = f.read(&mut buf) {
        println!("Read bytes len: {}", read_bytes);
    }
}

A good PR would be to get PAGE_SIZE in windows as well as we did for unix. Could you do it please?

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

@iddm iddm changed the title Windows Add windows os support Oct 29, 2018
@iddm iddm mentioned this pull request Mar 20, 2019
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.

2 participants