[determinator] initial Windows support

This seems to work but I haven't managed to test it very much.

.github/workflows/ci.yml

@@ -46,9 +46,8 @@ jobs:
     runs-on: ${{ matrix.os }}
     strategy:
       matrix:
-        # TODO: Run these tests on windows-latest. There are some issues around path separators:
-        # https://github.com/facebookincubator/cargo-guppy/actions/runs/303264009
-        os: [ ubuntu-latest, macos-latest ]
+        os: [ ubuntu-latest, macos-latest, windows-latest ]
+      fail-fast: false
     env:
       RUSTFLAGS: -D warnings
     steps:
@@ -116,7 +115,8 @@ jobs:
     runs-on: ${{ matrix.os }}
     strategy:
       matrix:
-        os: [ ubuntu-latest, macos-latest ]
+        os: [ ubuntu-latest, macos-latest, windows-latest ]
+      fail-fast: false
     env:
       RUSTFLAGS: -D warnings
     steps:
@@ -146,7 +146,10 @@ jobs:
     runs-on: ${{ matrix.os }}
     strategy:
       matrix:
+        # Don't run cargo-compare tests on Windows for now. See
+        # https://github.com/facebookincubator/cargo-guppy/issues/265.
         os: [ ubuntu-latest, macos-latest ]
+      fail-fast: false
     env:
       RUSTFLAGS: -D warnings
       PROPTEST_MULTIPLIER: 64

fixtures/README.md

@@ -6,6 +6,7 @@ The fixtures are organized into several folders.
 
 ## `cargo metadata` output
 
+* `determinator-paths`: determinator path matching across platforms.
 * `small`: relatively simple examples that cover basic and some edge case functionality
 * `large`: complex examples pulled from real-world Rust repositories, that test a variety of edge cases
 * `invalid`: examples that are [*representable*](https://oleb.net/blog/2018/03/making-illegal-states-unrepresentable/)

fixtures/determinator-paths/README.md

@@ -0,0 +1,7 @@
+# determinator paths fixtures
+
+This fixture is used to test that path matching works correctly across platforms.
+
+* `git-diff.out`: The output of `git diff -z --name-only f9ddae14671073f9fe847f8c6190de596f87a119^ f9ddae14671073f9fe847f8c6190de596f87a119`, identical on Windows and Linux.
+* `guppy-win.json`: `cargo metadata` output on Windows.
+* `guppy-linux.json`: `cargo metadata` output on Linux.

fixtures/src/json.rs

@@ -213,6 +213,9 @@ impl JsonFixture {
         // rel_path is relative to this dir.
         let mut abs_path = fixtures_dir.join("src");
         abs_path.push(rel_path);
+        let abs_path = abs_path
+            .canonicalize()
+            .expect("fixture path canonicalization succeeded");
 
         let workspace_root = fixtures_dir.parent().expect("up to workspace root");
         let workspace_path = pathdiff::diff_paths(&abs_path, workspace_root)

internal-tools/cargo-compare/src/tests/proptest_helpers.rs

@@ -13,7 +13,10 @@ macro_rules! proptest_suite {
             use crate::tests::proptest_helpers::*;
             use proptest::prelude::*;
 
+            // Ignore cargo-compare tests on Windows for now. See
+            // https://github.com/facebookincubator/cargo-guppy/issues/265.
             #[test]
+            #[cfg_attr(windows, ignore)]
             fn proptest_compare() {
                 let fixture = Fixture::$name();
                 // cargo is pretty slow, so limit the number of test cases.

tools/determinator/CHANGELOG.md

@@ -1,5 +1,11 @@
 # Changelog
 
+## [0.2.1] - 2021-02-04
+
+### Added
+
+* Experimental Windows support. There may still be bugs around path normalization: please [report them](https://github.com/facebookincubator/cargo-guppy/issues/new).
+
 ## [0.2.0] - 2021-02-03
 
 ### Changed
@@ -24,6 +30,7 @@ Initial release.
 * Path-based and package-based custom rules, including a default set of rules for files like `rust-toolchain` and `Cargo.lock`.
 * A `Paths0` wrapper to make it easier to retrieve changes from source control.
 
+[0.2.1]: https://github.com/facebookincubator/cargo-guppy/releases/tag/determinator-0.2.1
 [0.2.0]: https://github.com/facebookincubator/cargo-guppy/releases/tag/determinator-0.2.0
 [0.1.1]: https://github.com/facebookincubator/cargo-guppy/releases/tag/determinator-0.1.1
 [0.1.0]: https://github.com/facebookincubator/cargo-guppy/releases/tag/determinator-0.1.0

tools/determinator/Cargo.toml

@@ -1,6 +1,6 @@
 [package]
 name = "determinator"
-version = "0.2.0"
+version = "0.2.1"
 description = "Figure out which packages changed between two commits to a workspace."
 documentation = "https://docs.rs/determinator"
 authors = ["Rain <[email protected]>"]
@@ -27,6 +27,7 @@ include = [
 ]
 
 [dependencies]
+cfg-if = "1.0.0"
 globset = "0.4.6"
 guppy = { version = "0.7.0", path = "../../guppy", features = ["rayon1", "summaries"] }
 itertools = "0.10"

tools/determinator/README.md

@@ -42,6 +42,12 @@ for package in determinator_set.affected_set.packages(DependencyDirection::Forwa
 }
 ```
 
+## Platform support
+
+* **Unix platforms**: The determinator works and is supported.
+* **Windows**: experimental support. There may still be bugs around path normalization: please
+  [report them](https://github.com/facebookincubator/cargo-guppy/issues/new)!
+
 ## How it works
 
 A Rust package can behave differently if one or more of the following change:

tools/determinator/src/lib.rs

@@ -43,6 +43,12 @@
 //! }
 //! ```
 //!
+//! # Platform support
+//!
+//! * **Unix platforms**: The determinator works and is supported.
+//! * **Windows**: experimental support. There may still be bugs around path normalization: please
+//!   [report them](https://github.com/facebookincubator/cargo-guppy/issues/new)!
+//!
 //! # How it works
 //!
 //! A Rust package can behave differently if one or more of the following change:

tools/determinator/src/paths0.rs

@@ -1,7 +1,8 @@
 // Copyright (c) The cargo-guppy Contributors
 // SPDX-License-Identifier: MIT OR Apache-2.0
 
-use std::{ffi::OsStr, path::Path, str::Utf8Error};
+use cfg_if::cfg_if;
+use std::{path::Path, str::Utf8Error};
 
 /// A store for null-separated paths.
 ///
@@ -83,40 +84,47 @@ impl Paths0 {
     /// * the `Utf8Error` that it returned.
     pub fn new(buf: impl Into<Vec<u8>>) -> Result<Self, (Vec<u8>, Utf8Error)> {
         let buf = buf.into();
-        if cfg!(unix) {
-            return Ok(Self::new_unix(buf));
+        cfg_if! {
+            if #[cfg(unix)] {
+                Ok(Self::new_unix(buf))
+            } else {
+                // Validate UTF-8.
+                Self::validate_utf8(&buf)?;
+                Ok(Self::strip_trailing_null_byte(buf))
+            }
         }
-
-        // Validate UTF-8.
-        Self::validate_utf8(&buf)?;
-
-        Ok(Self::strip_trailing_null_byte(buf))
     }
 
     /// Creates a new instance of `Paths0`, converting `/` to `\` on platforms like Windows.
     ///
     /// Some tools like Git (but not Mercurial) return paths with `/` on Windows, even though the
     /// canonical separator on the platform is `\`. This constructor changes all instances of `/`
     /// to `\`.
+    ///
+    /// This method is available on Unix and Windows platforms.
+    #[cfg(any(unix, windows))]
     pub fn new_forward_slashes(buf: impl Into<Vec<u8>>) -> Result<Self, (Vec<u8>, Utf8Error)> {
-        let mut buf = buf.into();
-        if cfg!(unix) {
-            return Ok(Self::new_unix(buf));
-        }
-
-        // Validate UTF-8.
-        Self::validate_utf8(&buf)?;
+        cfg_if! {
+            if #[cfg(unix)] {
+                Ok(Self::new_unix(buf.into()))
+            }
+            else {
+                let mut buf = buf.into();
+                // Validate UTF-8.
+                Self::validate_utf8(&buf)?;
 
-        // Change all `/` to `\` on Windows.
-        if std::path::MAIN_SEPARATOR == '\\' {
-            buf.iter_mut().for_each(|b| {
-                if *b == b'/' {
-                    *b = b'\\';
+                // Change all `/` to `\` on Windows.
+                if std::path::MAIN_SEPARATOR == '\\' {
+                    buf.iter_mut().for_each(|b| {
+                        if *b == b'/' {
+                            *b = b'\\';
+                        }
+                    });
                 }
-            });
-        }
 
-        Ok(Self::strip_trailing_null_byte(buf))
+                Ok(Self::strip_trailing_null_byte(buf))
+            }
+        }
     }
 
     /// Creates a new instance of `Paths0` on Unix platforms.
@@ -136,6 +144,7 @@ impl Paths0 {
     // Helper methods
     // ---
 
+    #[cfg(not(unix))]
     fn validate_utf8(buf: &[u8]) -> Result<(), (Vec<u8>, Utf8Error)> {
         buf.split(|b| *b == 0)
             .try_for_each(|path| match std::str::from_utf8(path) {
@@ -163,21 +172,24 @@ impl<'a> IntoIterator for &'a Paths0 {
             return Box::new(std::iter::empty());
         }
 
-        if cfg!(unix) {
-            // Paths are arbitrary byte sequences on Unix.
-            use std::os::unix::ffi::OsStrExt;
-
-            Box::new(
-                self.buf
-                    .split(|b| *b == 0)
-                    .map(|path| Path::new(OsStr::from_bytes(path))),
-            )
-        } else {
-            Box::new(self.buf.split(|b| *b == 0).map(|path| {
-                let s = std::str::from_utf8(path)
-                    .expect("buf constructor should have already validated this path as UTF-8");
-                Path::new(s)
-            }))
+        cfg_if! {
+            if #[cfg(unix)] {
+                // Paths are arbitrary byte sequences on Unix.
+                use std::ffi::OsStr;
+                use std::os::unix::ffi::OsStrExt;
+
+                Box::new(
+                    self.buf
+                        .split(|b| *b == 0)
+                        .map(|path| Path::new(OsStr::from_bytes(path))),
+                )
+            } else {
+                Box::new(self.buf.split(|b| *b == 0).map(|path| {
+                    let s = std::str::from_utf8(path)
+                        .expect("buf constructor should have already validated this path as UTF-8");
+                    Path::new(s)
+                }))
+            }
         }
     }
 }
@@ -208,6 +220,25 @@ mod tests {
         );
     }
 
+    // This is really a Windows test but it should work on all platforms.
+    #[test]
+    fn backslashes() {
+        paths_eq(*b"a\\b\\c", &["a\\b\\c"]);
+        paths_eq(*b"a\\b\0a\\c", &["a\\b", "a\\c"]);
+        paths_eq(*b"a\\b\0a\\c\0", &["a\\b", "a\\c"]);
+    }
+
+    #[cfg(windows)]
+    #[test]
+    fn forward_slashes() {
+        paths_eq_fwd(*b"a/b/c", &["a\\b\\c"]);
+        paths_eq(*b"a/b\0a/c", &["a\\b", "a\\c"]);
+        paths_eq(*b"a/b\0a/c\0", &["a\\b", "a\\c"]);
+
+        // Also test mixed forward/backslashes.
+        paths_eq(*b"a/b\0a\\c", &["a\\b", "a\\c"]);
+    }
+
     fn paths_eq(bytes: impl Into<Vec<u8>>, expected: &[&str]) {
         let paths = Paths0::new(bytes.into()).expect("null-separated paths are valid");
         let actual: Vec<_> = paths.iter().collect();
@@ -219,7 +250,7 @@ mod tests {
     #[cfg(unix)]
     fn paths_eq_bytes(bytes: impl Into<Vec<u8>>, expected: &[&[u8]]) {
         // Paths are arbitrary byte sequences on Unix.
-        use std::os::unix::ffi::OsStrExt;
+        use std::{ffi::OsStr, os::unix::ffi::OsStrExt};
 
         let paths = Paths0::new(bytes.into()).expect("null-separated paths are valid");
         let actual: Vec<_> = paths.iter().collect();
@@ -230,4 +261,14 @@ mod tests {
 
         assert_eq!(actual, expected, "paths match");
     }
+
+    #[cfg(windows)]
+    fn paths_eq_fwd(bytes: impl Into<Vec<u8>>, expected: &[&str]) {
+        let paths =
+            Paths0::new_forward_slashes(bytes.into()).expect("null-separated paths are valid");
+        let actual: Vec<_> = paths.iter().collect();
+        let expected: Vec<_> = expected.iter().map(Path::new).collect();
+
+        assert_eq!(actual, expected, "paths match");
+    }
 }