Implement _removeItem in terms of DirectoryEnumerator #2002

gmittert · 2019-03-14T18:42:23Z

Alternate implementation to #1976 that uses DirectoryEnumerator

millenomi · 2019-03-14T19:19:29Z

millenomi · 2019-03-14T19:20:07Z

Foundation/FileManager.swift

-                    fts_close(stream)
+                if (URL(fileURLWithPath: itemPath).hasDirectoryPath) {
+                  #if os(Windows)
+                  let result = _NS_rmdir(itemPath)


Do we want to just fileprivate let _NS_rmdir = rmdir on non-Windows?

And similarly for _NS_unlink.

Or fileprivate let rmdir = _NS_rundir on Windows?

compnerd · 2019-03-14T19:51:25Z

Foundation/FileManager.swift

+                                            return _NS_unlink(path) != 0
+                                            #else
+                                            return unlink(path) != 0
+                                            #endif


This can also be unlink(path) now right?

spevans · 2019-03-14T20:59:51Z

Wont this spend a lot of effort to convert every file entry from file system representation into a URL then convert it back again to remove it? Im not sure of the benefit of this change.

gmittert · 2019-03-14T21:52:24Z

Wont this spend a lot of effort to convert every file entry from file system representation into a URL then convert it back again to remove it? Im not sure of the benefit of this change.

It does add extra URL calls, however, it's one less use of fts and means the same codepath works across Windows/Darwin/Linux. How expensive is converting to a URL? I'm still okay with #1976 if adding an extra conversion is too expensive to be worth it.

compnerd · 2019-03-14T21:58:27Z

Could also do the .standardizingPath to avoid the URL conversions.

millenomi · 2019-03-15T16:53:54Z

@swift-ci please test

gmittert · 2019-03-15T20:01:05Z

Could also do the .standardizingPath to avoid the URL conversions.

I can remove all the URL conversions in _removeItem, but the issue is still that NSURLDirectoryEnumerator gives back URLs rather than Strings so it will still be doing the conversion there and back.

compnerd · 2019-03-15T21:14:11Z

Yes, but that is a feature since the paths need to be converted to FSRs prior to the unlink.

millenomi · 2019-03-15T21:28:42Z

@swift-ci please test

gmittert · 2019-03-15T21:33:42Z

Okay, I've removed the unneeded URL conversions in _removeItem, the only one remaining is the needed one to create the Enumerator.

compnerd · 2019-03-17T03:07:08Z

@swift-ci please test

compnerd · 2019-03-19T16:05:24Z

@swift-ci please test

gmittert · 2019-03-19T19:51:39Z

It fails when creating a symlink for swiftpm... I don't think it's related?

12:45:08 [37/38] Compiling Swift Module 'SwiftPMPackageTests' (1 sources)
12:45:08 error: symlink error: File exists (17): /home/buildnode/jenkins/workspace/swift-corelibs-foundation-PR-Linux/buildbot_linux/swiftpm-linux-x86_64/release -> x86_64-unknown-linux/release
12:45:08 --- bootstrap: error: build failed with exit status 1
12:45:08 Building the standard library for: swift-stdlib-linux-x86_64
12:45:08 Running Swift tests for: check-swift-all-linux-x86_64 check-swift-all-optimize-linux-x86_64
12:45:08 /home/buildnode/jenkins/workspace/swift-corelibs-foundation-PR-Linux/swift/utils/build-script: fatal error: command terminated with a non-zero exit status 1, aborting

compnerd · 2019-03-20T17:08:16Z

@swift-ci please test

compnerd · 2019-03-27T03:33:26Z

@swift-ci please test

gmittert · 2019-03-27T03:36:32Z

@swift-ci please test

Ah hold up a bit, this is actually broken on Linux, I've repro'd it locally. Just having issues setting up a Linux environment where I can test it properly.

spevans · 2019-03-27T07:48:11Z

You are calling the unlink and rmdir without using the FSR of the file name.
Also every _fileExists call does at least one lstat call and possibly a second stat call making this quite system call heavy.

Another point regarding the FSR, is that the fts_read already returns the filename in FSR which can be passed straight to unlink and rmdir instead of converting the name to a String and then back again.

I think this approach in general is wrong, for an internal helper like this it should just use the best per-OS solution to delete a file or directory structure.

gmittert · 2019-03-27T15:48:50Z

I think this approach in general is wrong, for an internal helper like this it should just use the best per-OS solution to delete a file or directory structure.

I actually agree, and I think I'd rather do #1976 instead. @compnerd if we really care about a single codepath, one other alternative would be to have an fts shim and implement that on Windows and call through to the real thing on everything else, but that might be more effort than it's worth.

compnerd · 2019-03-27T16:35:46Z

I don't think that we want to re-implement FTS on top of the Windows APIs. Would it not be possible to actually have an internal method to avoid the overheads of the FTS translation and the stating? If not, then I suppose that we can split the file into two and have a FileManager+POSIX.swift and FileManager+Win32.swift and start duplicating the interfaces.

gmittert · 2019-04-15T17:57:00Z

Closing in favour of #2117

gmittert mentioned this pull request Mar 14, 2019

Implement removeItem on Windows #1976

Closed

millenomi reviewed Mar 14, 2019

View reviewed changes

gmittert force-pushed the TreeRemovalByIterator branch from 9c8bfdc to 99ec935 Compare March 14, 2019 19:44

compnerd reviewed Mar 14, 2019

View reviewed changes

gmittert force-pushed the TreeRemovalByIterator branch from 99ec935 to 591661f Compare March 14, 2019 19:55

Implement _removeItem in terms of DirectoryEnumerator

5114418

gmittert force-pushed the TreeRemovalByIterator branch from 591661f to 5114418 Compare March 15, 2019 21:31

gmittert mentioned this pull request Apr 2, 2019

Split FileManager into Platform Dependent Files #2057

Merged

gmittert closed this Apr 15, 2019

gmittert deleted the TreeRemovalByIterator branch April 15, 2019 17:57

Implement _removeItem in terms of DirectoryEnumerator #2002

Implement _removeItem in terms of DirectoryEnumerator #2002

Uh oh!

Conversation

gmittert commented Mar 14, 2019

Uh oh!

millenomi commented Mar 14, 2019

Uh oh!

millenomi Mar 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

millenomi Mar 14, 2019

Choose a reason for hiding this comment

Uh oh!

compnerd Mar 14, 2019

Choose a reason for hiding this comment

Uh oh!

gmittert Mar 14, 2019

Choose a reason for hiding this comment

Uh oh!

compnerd Mar 14, 2019

Choose a reason for hiding this comment

Uh oh!

gmittert Mar 14, 2019

Choose a reason for hiding this comment

Uh oh!

spevans commented Mar 14, 2019

Uh oh!

gmittert commented Mar 14, 2019

Uh oh!

compnerd commented Mar 14, 2019

Uh oh!

millenomi commented Mar 15, 2019

Uh oh!

gmittert commented Mar 15, 2019

Uh oh!

compnerd commented Mar 15, 2019

Uh oh!

millenomi commented Mar 15, 2019

Uh oh!

gmittert commented Mar 15, 2019

Uh oh!

compnerd commented Mar 17, 2019

Uh oh!

compnerd commented Mar 19, 2019

Uh oh!

gmittert commented Mar 19, 2019

Uh oh!

compnerd commented Mar 20, 2019

Uh oh!

compnerd commented Mar 27, 2019

Uh oh!

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

Uh oh!

Uh oh!

spevans commented Mar 27, 2019

Uh oh!

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

Uh oh!

Uh oh!

compnerd commented Mar 27, 2019

Uh oh!

gmittert commented Apr 15, 2019

Uh oh!

Uh oh!

millenomi Mar 14, 2019 •

edited

Loading

gmittert commented Mar 27, 2019 •

edited

Loading

gmittert commented Mar 27, 2019 •

edited

Loading