davidMcneil / mnist

MNIST data set parser https://crates.io/crates/mnist
20 stars 9 forks source link

Adds conditional flag for unix vs. Windows file metadata #11

Closed quietlychris closed 2 years ago

quietlychris commented 2 years ago

This is a preliminary fix for #10 by adding a conditional compilation flag for the target_family between unix and windows. Please don't merge until it's been verified that this actually fixes the issue on Windows (another contributor has volunteered to do that on their machine)

relf commented 2 years ago

Feedback on this PR when compiling on Windows

(base) D:\rlafage\workspace\mnist>cargo build --features download
   Compiling mnist v0.5.0 (D:\rlafage\workspace\mnist)
warning: unused import: `std::os::windows::fs::MetadataExt`
  --> src\download.rs:89:17
   |
89 |             use std::os::windows::fs::MetadataExt;
   |                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: `#[warn(unused_imports)]` on by default

warning: use of deprecated function `std::thread::sleep_ms`: replaced by `std::thread::sleep`
   --> src\download.rs:103:29
    |
103 |                     thread::sleep_ms(10);
    |                             ^^^^^^^^
    |
    = note: `#[warn(deprecated)]` on by default

error[E0599]: no method named `size` found for struct `Metadata` in the current scope
   --> src\download.rs:101:41
    |
101 |                     current_size = meta.size() as usize;
    |                                         ^^^^ method not found in `Metadata`

For more information about this error, try `rustc --explain E0599`.
warning: `mnist` (lib) generated 2 warnings
error: could not compile `mnist` due to previous error; 2 warnings emitted

If I remove the download feature, I get:

(base) D:\rlafage\workspace\mnist>cargo build
   Compiling mnist v0.5.0 (D:\rlafage\workspace\mnist)
warning: unused variable: `base_url`
   --> src\lib.rs:404:17
    |
404 |             let base_url = if self.use_fashion_data {
    |                 ^^^^^^^^ help: if this is intentional, prefix it with an underscore: `_base_url`
    |
    = note: `#[warn(unused_variables)]` on by default

warning: panic message is not a string literal
   --> src\lib.rs:433:13
    |
433 | /             format!(
434 | |                 "Total data set length ({}) greater than maximum possible length ({}).",
435 | |                 total_length, available_length
436 | |             )
    | |_____________^
    |
    = note: `#[warn(non_fmt_panics)]` on by default
    = note: this usage of assert!() is deprecated; it will be a hard error in Rust 2021
    = note: for more information, see <https://doc.rust-lang.org/nightly/edition-guide/rust-2021/panic-macro-consistency.html>
    = note: the assert!() macro supports formatting, so there's no need for the format!() macro here
help: remove the `format!(..)` macro call
    |
433 ~
434 |                 "Total data set length ({}) greater than maximum possible length ({}).",
435 |                 total_length, available_length
436 ~
    |

warning: panic message is not a string literal
   --> src\lib.rs:562:9
    |
562 | /         format!(
563 | |             "Expected magic number {} got {}.",
564 | |             LBL_MAGIC_NUMBER, magic_number
565 | |         )
    | |_________^
    |
    = note: this usage of assert!() is deprecated; it will be a hard error in Rust 2021
    = note: for more information, see <https://doc.rust-lang.org/nightly/edition-guide/rust-2021/panic-macro-consistency.html>
    = note: the assert!() macro supports formatting, so there's no need for the format!() macro here
help: remove the `format!(..)` macro call
    |
562 ~
563 |             "Expected magic number {} got {}.",
564 |             LBL_MAGIC_NUMBER, magic_number
565 ~
    |

warning: panic message is not a string literal
   --> src\lib.rs:572:9
    |
572 | /         format!(
573 | |             "Expected data set length of {} got {}.",
574 | |             expected_length, length
575 | |         )
    | |_________^
    |
    = note: this usage of assert!() is deprecated; it will be a hard error in Rust 2021
    = note: for more information, see <https://doc.rust-lang.org/nightly/edition-guide/rust-2021/panic-macro-consistency.html>
    = note: the assert!() macro supports formatting, so there's no need for the format!() macro here
help: remove the `format!(..)` macro call
    |
572 ~
573 |             "Expected data set length of {} got {}.",
574 |             expected_length, length
575 ~
    |

warning: panic message is not a string literal
   --> src\lib.rs:599:9
    |
599 | /         format!(
600 | |             "Expected magic number {} got {}.",
601 | |             IMG_MAGIC_NUMBER, magic_number
602 | |         )
    | |_________^
    |
    = note: this usage of assert!() is deprecated; it will be a hard error in Rust 2021
    = note: for more information, see <https://doc.rust-lang.org/nightly/edition-guide/rust-2021/panic-macro-consistency.html>
    = note: the assert!() macro supports formatting, so there's no need for the format!() macro here
help: remove the `format!(..)` macro call
    |
599 ~
600 |             "Expected magic number {} got {}.",
601 |             IMG_MAGIC_NUMBER, magic_number
602 ~
    |

warning: panic message is not a string literal
   --> src\lib.rs:609:9
    |
609 | /         format!(
610 | |             "Expected data set length of {} got {}.",
611 | |             expected_length, length
612 | |         )
    | |_________^
    |
    = note: this usage of assert!() is deprecated; it will be a hard error in Rust 2021
    = note: for more information, see <https://doc.rust-lang.org/nightly/edition-guide/rust-2021/panic-macro-consistency.html>
    = note: the assert!() macro supports formatting, so there's no need for the format!() macro here
help: remove the `format!(..)` macro call
    |
609 ~
610 |             "Expected data set length of {} got {}.",
611 |             expected_length, length
612 ~
    |

warning: panic message is not a string literal
   --> src\lib.rs:620:9
    |
620 |         format!("Expected rows length of {} got {}.", ROWS, rows)
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: this usage of assert!() is deprecated; it will be a hard error in Rust 2021
    = note: for more information, see <https://doc.rust-lang.org/nightly/edition-guide/rust-2021/panic-macro-consistency.html>
    = note: the assert!() macro supports formatting, so there's no need for the format!() macro here
help: remove the `format!(..)` macro call
    |
620 -         format!("Expected rows length of {} got {}.", ROWS, rows)
620 +         "Expected rows length of {} got {}.", ROWS, rows
    |

warning: panic message is not a string literal
   --> src\lib.rs:628:9
    |
628 |         format!("Expected cols length of {} got {}.", COLS, cols)
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: this usage of assert!() is deprecated; it will be a hard error in Rust 2021
    = note: for more information, see <https://doc.rust-lang.org/nightly/edition-guide/rust-2021/panic-macro-consistency.html>
    = note: the assert!() macro supports formatting, so there's no need for the format!() macro here
help: remove the `format!(..)` macro call
    |
628 -         format!("Expected cols length of {} got {}.", COLS, cols)
628 +         "Expected cols length of {} got {}.", COLS, cols
    |

warning: unused return value of `Vec::<T, A>::split_off` that must be used
   --> src\lib.rs:460:9
    |
460 |         tst_img.split_off(tst_len * ROWS * COLS);
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: `#[warn(unused_must_use)]` on by default
    = note: use `.truncate()` if you don't need the other half

warning: unused return value of `Vec::<T, A>::split_off` that must be used
   --> src\lib.rs:461:9
    |
461 |         tst_lbl.split_off(tst_len);
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: use `.truncate()` if you don't need the other half

warning: `mnist` (lib) generated 10 warnings
    Finished dev [unoptimized + debuginfo] target(s) in 0.40s
quietlychris commented 2 years ago

Okay, maybe this one will fix it? I think Windows uses file_size() for metadata, not simply size(). I should really buy a cheap Windows machine at some point just to have one around...

relf commented 2 years ago

Unfortunatly I still get an error, now I get:

(base) D:\rlafage\workspace\mnist>cargo build --features download
   Compiling mnist v0.5.0 (D:\rlafage\workspace\mnist)
error[E0658]: attributes on expressions are experimental
   --> src\download.rs:102:21
    |
102 |                     #[cfg(target_family = "unix")]
    |                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: see issue #15701 <https://github.com/rust-lang/rust/issues/15701> for more information

error: removing an expression is not supported in this position
   --> src\download.rs:102:21
    |
102 |                     #[cfg(target_family = "unix")]
    |                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error[E0658]: attributes on expressions are experimental
   --> src\download.rs:104:21
    |
104 |                     #[cfg(target_family = "windows")]
    |                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: see issue #15701 <https://github.com/rust-lang/rust/issues/15701> for more information

error: removing an expression is not supported in this position
   --> src\download.rs:104:21
    |
104 |                     #[cfg(target_family = "windows")]
    |                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

warning: use of deprecated function `std::thread::sleep_ms`: replaced by `std::thread::sleep`
   --> src\download.rs:108:29
    |
108 |                     thread::sleep_ms(10);
    |                             ^^^^^^^^
    |
    = note: `#[warn(deprecated)]` on by default

error[E0599]: no method named `size` found for struct `Metadata` in the current scope
   --> src\download.rs:103:41
    |
103 |                     current_size = meta.size() as usize;
    |                                         ^^^^ method not found in `Metadata`

Some errors have detailed explanations: E0599, E0658.
For more information about an error, try `rustc --explain E0599`.
warning: `mnist` (lib) generated 1 warning
error: could not compile `mnist` due to 5 previous errors; 1 warning emitted

I do not know if this because I use rust 1.57. Anyway I made it pass, by putting use declarations of MetadataExt at the top, create the two versions of file_size like that:

#[cfg(target_family = "unix")]
fn file_size(meta: &MetadataExt) -> usize {
    meta.size() as usize
}

#[cfg(target_family = "windows")]
fn file_size(meta: &MetadataExt) -> usize {
    meta.file_size() as usize
}

and then call

current_size = file_size(&meta);
relf commented 2 years ago

Ok, it compiles successfully on Windows. Thanks!

quietlychris commented 2 years ago

@davidMcneil If you wouldn't mind reviewing and potentially merging this whenever you get a chance, I'd appreciate it! As before, happy to discuss any changes you might think are necessary, although this is should be a pretty minor bug fix.