Windows test (#669)

* Windows test

* Fixces

* Add s3 path formatting test

* Use simple format method for prefix paths in s3

* Add back safety

---------

Co-authored-by: Matthew Iannucci <matthew@earthmover.io>

Author: paraseba

.github/workflows/windows-check.yml

@@ -0,0 +1,54 @@
+name: Windows tests
+on:
+  pull_request:
+    types: [opened, reopened, synchronize, labeled]
+  push:
+    branches:
+      - main
+
+env:
+  CARGO_INCREMENTAL: 0
+  CARGO_NET_RETRY: 10
+  CI: 1
+  RUST_BACKTRACE: short
+  RUSTFLAGS: "-D warnings -W unreachable-pub -W bare-trait-objects"
+  RUSTUP_MAX_RETRIES: 10
+
+jobs:
+  rust:
+    name: Run rust tests in windows
+    timeout-minutes: 20
+    runs-on: windows-latest
+    #defaults:
+    #  run:
+    #    working-directory: ./
+    #permissions:
+      #contents: read
+      #actions: read
+      #pull-requests: read
+    env:
+      #CC: deny_c
+      RUST_CHANNEL: 'stable'
+
+
+    steps:
+      - name: Checkout repository
+        uses: actions/checkout@v4
+        with:
+          ref: ${{ github.event.pull_request.head.sha }}
+
+      - name: Install Rust toolchain
+        run: |
+          rustup update --no-self-update ${{ env.RUST_CHANNEL }}
+          rustup component add --toolchain ${{ env.RUST_CHANNEL }} rustfmt rust-src clippy
+          rustup default ${{ env.RUST_CHANNEL }}
+
+      - name: Cache Dependencies
+        uses: Swatinem/rust-cache@v2
+        with:
+          # workspaces: "rust -> target"
+          key: windows-${{ env.RUST_CHANNEL }}
+
+      - name: Check
+        run: |
+          cargo test --lib

icechunk/src/storage/mod.rs

@@ -712,10 +712,27 @@ pub async fn new_gcs_storage(
 #[allow(clippy::unwrap_used, clippy::panic)]
 mod tests {
 
-    use std::collections::HashSet;
+    use std::{collections::HashSet, fs::File, io::Write, path::PathBuf};
 
     use super::*;
     use proptest::prelude::*;
+    use tempfile::TempDir;
+
+    #[tokio::test]
+    async fn test_is_clean() {
+        let repo_dir = TempDir::new().unwrap();
+        let s = new_local_filesystem_storage(repo_dir.path()).await.unwrap();
+        assert!(s.root_is_clean().await.unwrap());
+
+        let mut file = File::create(repo_dir.path().join("foo.txt")).unwrap();
+        write!(file, "hello").unwrap();
+        assert!(!s.root_is_clean().await.unwrap());
+
+        let inside_existing =
+            PathBuf::from_iter([repo_dir.path().as_os_str().to_str().unwrap(), "foo"]);
+        let s = new_local_filesystem_storage(&inside_existing).await.unwrap();
+        assert!(s.root_is_clean().await.unwrap());
+    }
 
     proptest! {
         #![proptest_config(ProptestConfig {

icechunk/src/storage/object_store.rs

@@ -83,7 +83,6 @@ impl ObjectStorage {
             Arc::new(LocalFileSystemObjectStoreBackend { path: prefix.to_path_buf() });
         let client = backend.mk_object_store().await?;
         let storage = ObjectStorage { backend, client: OnceCell::new_with(Some(client)) };
-
         Ok(storage)
     }
 
@@ -1022,6 +1021,8 @@ mod tests {
 
     use tempfile::TempDir;
 
+    use crate::format::{ChunkId, ManifestId, SnapshotId};
+
     use super::ObjectStorage;
 
     #[tokio::test]
@@ -1071,4 +1072,34 @@ mod tests {
             ObjectStorage::new_local_filesystem(PathBuf::from(&rel_path).as_path()).await;
         assert!(store.is_ok());
     }
+
+    #[tokio::test]
+    async fn test_object_store_paths() {
+        let store = ObjectStorage::new_local_filesystem(PathBuf::from(".").as_path())
+            .await
+            .unwrap();
+
+        let ref_key = "ref_key";
+        let ref_path = store.ref_key(ref_key);
+        assert_eq!(ref_path.to_string(), format!("refs/{ref_key}"));
+
+        let snapshot_id = SnapshotId::random();
+        let snapshot_path = store.get_snapshot_path(&snapshot_id);
+        assert_eq!(snapshot_path.to_string(), format!("snapshots/{snapshot_id}"));
+
+        let manifest_id = ManifestId::random();
+        let manifest_path = store.get_manifest_path(&manifest_id);
+        assert_eq!(manifest_path.to_string(), format!("manifests/{manifest_id}"));
+
+        let chunk_id = ChunkId::random();
+        let chunk_path = store.get_chunk_path(&chunk_id);
+        assert_eq!(chunk_path.to_string(), format!("chunks/{chunk_id}"));
+
+        let transaction_id = SnapshotId::random();
+        let transaction_path = store.get_transaction_path(&transaction_id);
+        assert_eq!(
+            transaction_path.to_string(),
+            format!("transactions/{transaction_id}")
+        );
+    }
 }

icechunk/src/storage/s3.rs

@@ -30,7 +30,6 @@ use aws_sdk_s3::{
 use aws_smithy_types_convert::{date_time::DateTimeExt, stream::PaginationStreamExt};
 use bytes::{Buf, Bytes};
 use chrono::{DateTime, Utc};
-use err_into::ErrorInto as _;
 use futures::{
     stream::{self, BoxStream},
     StreamExt, TryStreamExt,
@@ -150,10 +149,10 @@ impl S3Storage {
 
     fn get_path_str(&self, file_prefix: &str, id: &str) -> StorageResult<String> {
         let path = PathBuf::from_iter([self.prefix.as_str(), file_prefix, id]);
-        path.into_os_string()
-            .into_string()
-            .map_err(StorageErrorKind::BadPrefix)
-            .err_into()
+        let path_str =
+            path.into_os_string().into_string().map_err(StorageErrorKind::BadPrefix)?;
+
+        Ok(path_str.replace("\\", "/"))
     }
 
     fn get_path<const SIZE: usize, T: FileTypeTag>(
@@ -187,10 +186,10 @@ impl S3Storage {
 
     fn ref_key(&self, ref_key: &str) -> StorageResult<String> {
         let path = PathBuf::from_iter([self.prefix.as_str(), REF_PREFIX, ref_key]);
-        path.into_os_string()
-            .into_string()
-            .map_err(StorageErrorKind::BadPrefix)
-            .err_into()
+        let path_str =
+            path.into_os_string().into_string().map_err(StorageErrorKind::BadPrefix)?;
+
+        Ok(path_str.replace("\\", "/"))
     }
 
     async fn get_object_reader(
@@ -612,10 +611,7 @@ impl Storage for S3Storage {
         _settings: &Settings,
         prefix: &str,
     ) -> StorageResult<BoxStream<'a, StorageResult<ListInfo<String>>>> {
-        let prefix = PathBuf::from_iter([self.prefix.as_str(), prefix])
-            .into_os_string()
-            .into_string()
-            .map_err(StorageErrorKind::BadPrefix)?;
+        let prefix = format!("{}/{}", self.prefix, prefix).replace("//", "/");
         let stream = self
             .get_client()
             .await
@@ -798,4 +794,39 @@ mod tests {
         let deserialized: S3Storage = serde_json::from_str(&serialized).unwrap();
         assert_eq!(storage.config, deserialized.config);
     }
+
+    #[tokio::test]
+    async fn test_s3_paths() {
+        let storage = S3Storage::new(
+            S3Options {
+                region: Some("us-west-2".to_string()),
+                endpoint_url: None,
+                allow_http: true,
+                anonymous: false,
+            },
+            "bucket".to_string(),
+            Some("prefix".to_string()),
+            S3Credentials::FromEnv,
+        )
+        .unwrap();
+
+        let ref_path = storage.ref_key("ref_key").unwrap();
+        assert_eq!(ref_path, "prefix/refs/ref_key");
+
+        let snapshot_id = SnapshotId::random();
+        let snapshot_path = storage.get_snapshot_path(&snapshot_id).unwrap();
+        assert_eq!(snapshot_path, format!("prefix/snapshots/{snapshot_id}"));
+
+        let manifest_id = ManifestId::random();
+        let manifest_path = storage.get_manifest_path(&manifest_id).unwrap();
+        assert_eq!(manifest_path, format!("prefix/manifests/{manifest_id}"));
+
+        let chunk_id = ChunkId::random();
+        let chunk_path = storage.get_chunk_path(&chunk_id).unwrap();
+        assert_eq!(chunk_path, format!("prefix/chunks/{chunk_id}"));
+
+        let transaction_id = SnapshotId::random();
+        let transaction_path = storage.get_transaction_path(&transaction_id).unwrap();
+        assert_eq!(transaction_path, format!("prefix/transactions/{transaction_id}"));
+    }
 }