skade / leveldb-sys

MIT License
7 stars 15 forks source link

Windows isn’t supported #2

Closed chris-morgan closed 3 years ago

chris-morgan commented 7 years ago

Firstly, it’s depending on std::os::unix which isn’t available on Windows:

error[E0432]: unresolved import `std::os::unix::fs::PermissionsExt`
 --> src/build.rs:6:5
  |
6 | use std::os::unix::fs::PermissionsExt;
  |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Could not find `unix` in `std::os`

error: no method named `mode` found for type `std::fs::Permissions` in the current scope
   --> src/build.rs:173:30
    |
173 |     let current_mode = perms.mode();
    |                              ^^^^

error: no method named `set_mode` found for type `std::fs::Permissions` in the current scope
   --> src/build.rs:174:11
    |
174 |     perms.set_mode(0o100 | current_mode);
    |           ^^^^^^^^

error: aborting due to 2 previous errors

Setting +x isn’t relevant on Windows, so that can just be stubbed out on Windows:

diff --git a/src/build.rs b/src/build.rs
index 81eb58d..c239317 100644
--- a/src/build.rs
+++ b/src/build.rs
@@ -3,6 +3,7 @@ use std::ffi::OsString;
 use std::fs::File;
 use std::fs;
 use std::env;
+#[cfg(unix)]
 use std::os::unix::fs::PermissionsExt;
 use std::io::{Write,BufReader};
 use std::io::BufRead;
@@ -167,12 +168,21 @@ fn main() {
         println!("[build] Patching complete");
     }

-    // Set executable bits on the file
+    // Set executable bits on the file if appropriate (it's not on Windows)

-    let mut perms = fs::metadata(&detect_path).ok().expect("metadata missing").permissions();
-    let current_mode = perms.mode();
-    perms.set_mode(0o100 | current_mode);
-    fs::set_permissions(&detect_path, perms).ok().expect("permissions could not be set");
+    #[cfg(unix)]
+    fn set_exec(detect_path: &Path) {
+        let mut perms = fs::metadata(&detect_path).ok().expect("metadata missing").permissions();
+        let current_mode = perms.mode();
+        perms.set_mode(0o100 | current_mode);
+        fs::set_permissions(&detect_path, perms).ok().expect("permissions could not be set");
+    }
+
+    #[cfg(not(unix))]
+    fn set_exec(_detect_path: &Path) {
+    }
+
+    set_exec(&detect_path);

     // Build LevelDB
     build_leveldb(have_snappy);

But then the dependence on make crops up:

error: failed to run custom build command for `leveldb-sys v2.0.0 (file:///C:/code/leveldb-sys)`
process didn't exit successfully: `C:\code\leveldb-sys\target\debug\build\leveldb-sys-12748421a86593ec\build-script-build` (exit code: 101)
--- stdout
[build] Started
[build] Patching the `build_detect_platform` template
[build] Patching complete
[leveldb] Cleaning

--- stderr
thread 'main' panicked at 'clean failed', ../src/libcore\option.rs:705
note: Run with `RUST_BACKTRACE=1` for a backtrace.

In short, more work is needed to make it run smoothly on Windows.

I think the gcc crate is all about this sort of thing?

[Ported from https://github.com/andrew-d/leveldb-rs/issues/9 when I didn’t realise this had been split.]

chris-morgan commented 7 years ago

Oh, apparently LevelDB doesn’t currently support Windows. That’s really odd. I thought I heard mention of it being used in things like Chrome which definitely run on Windows.

skade commented 7 years ago

As far as I heard, Google does not use LevelDB much anymore anyways.

RocksDB and similar seem to be more popular.

On 20 Oct 2016, at 18:44, Chris Morgan notifications@github.com wrote:

Oh, apparently LevelDB doesn’t currently support Windows. That’s really odd. I thought I heard mention of it being used in things like Chrome which definitely run on Windows.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.

adumbidiot commented 4 years ago

I believe that leveldb has added support for Windows now, so would it be possible to reopen this issue?

skade commented 4 years ago

@adumbidiot patches definitely welcome!

Reference here: https://github.com/google/leveldb#building-for-windows

adumbidiot commented 4 years ago

Merging #14 is important since Windows support is only on newer versions of leveldb. However, it seems like the build system changed from gmake to cmake, which that pr seems to address. The pr also seems to remove all unix-specific functionality, so I think that pr should be sufficient for Windows support.

skade commented 4 years ago

@adumbidiot can I rely on you for windows testing? Then I'd do that.

adumbidiot commented 4 years ago

Sure. I just tested #14, and it seems to fail on windows when snappy support is enabled. The build script fails to find the compiled snappy lib as the windows linker doesn't understand the specified flag.

I believe this could be fixed by adding the windows linker flag to the LDFLAGS var, changing https://github.com/timberio/leveldb-sys/blob/update-leveldb/src/build.rs#L43 to something like format!("-L{dir} -libpath:{dir}", dir = snappy_prefix.join("lib").display()), though that seems kinda hacky.

skade commented 4 years ago

Hm, thanks. I have a windows machine available, I may check that.

Thanks for having a thorough look at that !

skade commented 3 years ago

Fixed by #18