Fix Up FileManager Tests on Windows #2277

gmittert · 2019-05-16T23:58:41Z

Windows directory enumerator now doesn't look in a directory it
returns until nextObject is called again
Fixed some path handling platform issues
Properly calculate level

gmittert · 2019-05-17T00:04:02Z

@spevans Is there a particular scenario you're testing for with the file names ending with . (https://github.com/apple/swift-corelibs-foundation/pull/2277/files#diff-fffbc0319b30adac9f635327a750b82fR509)? Windows isn't capable of create/working with files ending with . so I'm trying to figure out what I want to do with those test cases on Windows.

compnerd · 2019-05-17T00:31:01Z

Foundation/NSPathUtilities.swift

@@ -345,7 +349,11 @@ extension NSString {

        let automount = "/var/automount"
        resolved = resolved._tryToRemovePathPrefix(automount) ?? resolved
+#if os(Windows)
+        return String(resolved.map { $0 == "\\" ? "/" : $0 })


Why change the path separator here?

compnerd · 2019-05-17T00:34:01Z

Foundation/NSURL.swift

@@ -32,7 +32,11 @@ private func _standardizedPath(_ path: String) -> String {
    if !path.absolutePath {
        return path._nsObject.standardizingPath
    }
+    #if os(Windows)
+    return String(path.map({$0 == "/" ? "\\" : $0}))


My understanding is that the standardizing should collapse relative components (i.e. .. should be chased, . dropped). As such, any reason to not just use PathCchCanonicalizeEx?

compnerd · 2019-05-17T00:34:34Z

Foundation/NSPathUtilities.swift

@@ -118,7 +118,11 @@ extension String {
    }

    internal var absolutePath: Bool {


Bleh! It would be nice to rename this to isAbsolutePath.

compnerd · 2019-05-17T00:35:43Z

TestFoundation/TestFileManager.swift

                if item == "item" {
                    item1FileAttributes = e.fileAttributes
                } else if item == "path2/item" {
                    item2FileAttributes = e.fileAttributes
                }
+#endif


Isn't this hunk equivalent to just changing L459 (R) to } else if item == "path2/item" || item == "path2\\item" {?

compnerd · 2019-05-17T00:37:17Z

TestFoundation/TestFileManager.swift

            }
+#if os(Windows)
+            XCTAssertEqual(foundItems, Set(["item", "path2", "path2\\item"]))


I wonder if you can span the Set over two lines and do an #if os(Windows) to swap out the third element.

You can't. You can move the set creation elsewhere, or make it mutable and #if os(Windows) the addition.

compnerd · 2019-05-17T00:39:27Z

Foundation/FileManager+Win32.swift

+
+            func findFirstValidFile() -> URL? {
+              while let url = _stack.popLast() {
+                let dirFSR = url.withUnsafeFileSystemRepresentation { $0.flatMap { fsr in String(utf8String: fsr) } } ?? ""


Hmm, this seems pretty expensive.

compnerd · 2019-05-17T00:41:48Z

Foundation/FileManager+Win32.swift

+            func findFirstValidFile() -> URL? {
+              while let url = _stack.popLast() {
+                let dirFSR = url.withUnsafeFileSystemRepresentation { $0.flatMap { fsr in String(utf8String: fsr) } } ?? ""
+                if !FileManager.default.fileExists(atPath: dirFSR, isDirectory: nil) {


Huh? Should dirFSR be renamed? If isDirectory is nil, then we do not know if it is a file or directory, so it seems to be misleading.

compnerd · 2019-05-17T00:43:06Z

Foundation/FileManager+Win32.swift

-            return nil
+
+            var isDir: ObjCBool = false
+            let dirFSR = _current.withUnsafeFileSystemRepresentation { $0.flatMap { fsr in String(utf8String: fsr) } } ?? ""


Again, the dirFSR is really misleading here. Can we rename this as we do not know what the path points to.

gmittert · 2019-05-24T00:23:16Z

Cleaned up the directory enumerator code to be less terrible
Fixed some slash issues and removed the extra conversions

gmittert · 2019-05-24T00:23:38Z

@swift-ci please test

gmittert · 2019-05-24T18:54:52Z

@swift-ci please test

compnerd · 2019-05-24T19:06:59Z

CoreFoundation/URL.subproj/CFURL.c

@@ -4474,7 +4474,7 @@ CFStringRef CFURLCreateStringWithFileSystemPath(CFAllocatorRef allocator, CFURLR
    //	and do a linked-on-or-later check so we don't break third parties.
    //	See <rdar://problem/4003028> Converting volume name from POSIX to HFS form fails and
    //	<rdar://problem/4018895> CF needs to back out 4003028 for icky details.
-    if ( relPath && CFURLHasDirectoryPath(anURL) && CFStringGetLength(relPath) > 1 && CFStringGetCharacterAtIndex(relPath, CFStringGetLength(relPath)-1) == '/') {
+    if ( relPath && CFURLHasDirectoryPath(anURL) && CFStringGetLength(relPath) > 1 && CFStringGetCharacterAtIndex(relPath, CFStringGetLength(relPath)-1) == _CFGetSlash()) {


Since this is from a URL, is this change needed, and if it is from a URL is this not incorrect? CC: @millenomi

Seems like it's not, I've removed that change.

compnerd · 2019-05-24T19:11:27Z

TestFoundation/TestFileManager.swift

        let itemPath1 = basePath + "itemFile1"
+#if os(Windows)
+        // Filenames ending with '.' are not valid on Windows, so don't bother testing them


@millenomi - what do you think about this?

That's fine.

- Windows directory enumerator now doesn't look in a directory it returns until nextObject is called again - Fixed some path handling platform issues - Properly calculate level

compnerd

This looks good to me now.

@millenomi @spevans - any comments on the behavioural changes on Windows for FileManager?

gmittert · 2019-05-24T21:55:31Z

@swift-ci please test

millenomi · 2019-05-24T21:59:59Z

@swift-ci please test and merge

millenomi · 2019-05-24T22:00:10Z

@swift-ci please test and merge

gmittert requested a review from compnerd May 16, 2019 23:58

compnerd reviewed May 17, 2019

View reviewed changes

gmittert force-pushed the 94.5FixFM branch from 452388d to 7b2ca13 Compare May 24, 2019 00:21

gmittert force-pushed the 94.5FixFM branch from 7b2ca13 to 9c95f29 Compare May 24, 2019 18:54

compnerd reviewed May 24, 2019

View reviewed changes

Fix Up FileManager Tests on Windows

264ceae

- Windows directory enumerator now doesn't look in a directory it returns until nextObject is called again - Fixed some path handling platform issues - Properly calculate level

gmittert force-pushed the 94.5FixFM branch from 9c95f29 to 264ceae Compare May 24, 2019 19:23

compnerd approved these changes May 24, 2019

View reviewed changes

millenomi approved these changes May 24, 2019

View reviewed changes

compnerd merged commit c5357f3 into swiftlang:master May 26, 2019

gmittert deleted the 94.5FixFM branch June 10, 2019 21:25

swift-ci mentioned this pull request May 24, 2019

[SR-10738] TestFileManager.test_pathEnumerator #3340

Closed

		@@ -118,7 +118,11 @@ extension String {
		}

		internal var absolutePath: Bool {

Fix Up FileManager Tests on Windows #2277

Fix Up FileManager Tests on Windows #2277

Conversation

gmittert commented May 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gmittert commented May 17, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gmittert commented May 24, 2019

Uh oh!

gmittert commented May 24, 2019

Uh oh!

gmittert commented May 24, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

compnerd left a comment

Choose a reason for hiding this comment

Uh oh!

gmittert commented May 24, 2019

Uh oh!

millenomi commented May 24, 2019

Uh oh!

millenomi commented May 24, 2019

Uh oh!

Uh oh!

gmittert commented May 16, 2019 •

edited

Loading