SR-1005: Replace POSIX module by Foundation #284

Bouke · 2016-04-26T21:33:10Z

A partial implementation of SR-1005, replacing POSIX calls by Foundation calls. This PR addresses most file and directory operations by using NSFileManager and NSURL. To prevent a major overhaul of all usages of these calls, I kept all functions intact but changed the implementation. Some things to consider before merging this PR:

Resolved: segfaults on Linux (see comment below)
Cocoa Foundation API differs from SwiftFoundation. I've used #if os(Linux) for the interim until this is fixed.
Errors are now mostly Cocoa's default errors; so any code inspecting the error code should be adjusted (1 occurrence found and updated).
unlink and rmdir both use NSFileManager.removeItem(atPath:), which will remove files and directories recursively. The old implementations of unlink would only remove files, and rmdir would only remove directories non-recursive.
Temporary folders are now created with appending a GUID to guarantee random folder names.
mkdir returns relative folder name if relative name was passed. Use realpath(mkdir(...)) when old functionality is wanted; opt-in for absolute paths.
Unused functions (*time*, readlink, stat) were removed.
Dropped relativeTo argument of symlink as it was not used; all usages used an absolute path.

Not covered by this PR:

Replacing popen and system by NSTask, see SR-1005: [Foundation] Replace posix_spawn by NSTask #303
File handling (fopen fputs etc), see SR-1005: [Foundation] Use NSFileHandle instead of fopen #292

Dependencies:

Fix in NSFileManager.moveItem, see NSFileManager.moveItem - destination path should not exist before moving swift-corelibs-foundation#350

ddunbar · 2016-04-26T21:36:25Z

Awesome!

For test crashes, you should be able to run the tests in a debugger to see what is actually happening.

Can you file bugs for the places where the Foundation API differs across platforms?

It would be good to identify the source of the performance regression, as well.

Bouke · 2016-04-26T22:14:53Z

The segfault as reported by the LLDB debugger, however I'm not sure how to proceed.

* thread #1: tid = 13474, 0x00007ffff7b4b708 libswiftCore.so`swift_getErrorValue + 8, name = 'swift-build', stop reason = signal SIGSEGV: invalid address (fault address: 0x90)
    frame #0: 0x00007ffff7b4b708 libswiftCore.so`swift_getErrorValue + 8
libswiftCore.so`swift_getErrorValue:
->  0x7ffff7b4b708 <+8>:  movq   0x90(%rcx), %rcx
    0x7ffff7b4b70f <+15>: movzwl %cx, %esi
    0x7ffff7b4b712 <+18>: leaq   0x20(%rdi,%rsi), %rsi

API differences reported in SR-1328.

I suspect that the performance regressions are expected as the old POSIX calls are very lean. The new calls using Foundation probably contain extra error checking; more layers of indirection.

ddunbar · 2016-04-26T22:21:16Z

I suspect that the performance regressions are expected as the old POSIX calls are very lean. The new calls using Foundation probably contain extra error checking; more layers of indirection.

This probably isn't the case. While it is true there is a little more indirection, that is unlikely to be able to add up to 6% given the kind of work we are doing... my guess is something else is going on here. Can you try to identify a particular individual test which has regressed?

Bouke · 2016-04-27T07:32:57Z

This probably isn't the case. While it is true there is a little more indirection, that is unlikely to be able to add up to 6% given the kind of work we are doing... my guess is something else is going on here. Can you try to identify a particular individual test which has regressed?

That was indeed the case. Some NSFileManager functions performed even slightly better in a quick performance test. The full test suite is now run as fast as before.

mxcl · 2016-04-29T19:48:29Z

Great stuff. Let's deconflict and merge.

Bouke · 2016-04-30T18:53:20Z

Thanks! Indeed it does conflict, it was written against the latest development toolchain snapshot. The Foundation API is being imported now with new rules, so I'm unable to compile until I have an updated toolchain. I've yet to learn to build my own toolchain, maybe I'll just wait for a new toolchain snapshot.

Bouke · 2016-05-02T07:49:07Z

I've located the segfaults on Linux to be related to NSFileManager.rename(atPath:toPath:), for which I've filed a bug report and a PR swiftlang/swift-corelibs-foundation#350. I've updated this PR to have smaller atomic commits, which allowed for git bisect to find the segfault. The PR is rebased on master against the updated Foundation API.

Bouke · 2016-05-06T21:34:49Z

I've rebased the PR and verified against OSX and Ubuntu 15.10.

Bouke · 2016-05-07T06:32:34Z

I've also moved rename over to NSFileManager, making this PR dependent on the merger of swiftlang/swift-corelibs-foundation#350.

ddunbar · 2016-05-09T06:30:00Z

Can you rebase please?

ddunbar · 2016-05-09T08:09:05Z

@swift-ci please test and merge

ddunbar · 2016-05-09T08:11:53Z

Sources/Utility/unlink.swift

-
-import func libc.unlink
-import var libc.errno
+import Foundation

 public func unlink(_ path: String) throws {


This function should be renamed, using unlink as a synonym for removing a tree is confusing, as that is not the POSIX behavior.

ddunbar · 2016-05-09T15:08:18Z

@swift-ci please test

Bouke · 2016-05-09T16:52:53Z

Any ideas on why the build failed on OSX? The logs include the following test case, but I'm not sure how that's related?

ERROR: test_lldbmi_break_insert_function_pending (TestMiBreak.MiBreakTestCase)
   Test that 'lldb-mi --interpreter' works for pending function breakpoints.

ddunbar · 2016-05-09T16:57:13Z

That is unrelated, it sounds like from mail on swift-dev.

Bouke · 2016-05-10T04:47:24Z

Can you retest?

ddunbar · 2016-05-10T04:49:25Z

@swift-ci please test

aciidgh · 2016-05-11T06:51:52Z

@Bouke conflicts

Dropped relativeTo argument as it wasn't used.

The rmtree is not needed as directories are already removed recursively.

Bouke · 2016-05-11T12:24:19Z

@ddunbar @aciidb0mb3r rebased and tested against OSX and Ubuntu 15.10.

aciidgh · 2016-05-11T12:27:18Z

@swift-ci please test

ddunbar · 2016-05-19T07:31:13Z

I am going to start pulling over individual parts of this piecemeal for things that make more sense to be implemented on top of Foundation than POSIX wrappers...

ddunbar · 2016-05-19T09:05:34Z

I posted a new PR #355 which cherry picks (and reworks) a couple of commits from this PR.

The ones I took:

07f50bf [Foundation] Implemented mkdir
f5728c6 [Foundation] Implemented unlink / rmdir / rmtree

The ones I abandoned, and why:

3c673df [Foundation] Implemented POSIX.realpath

Foundation implementation is more complicated than POSIX.
24f2878 [Foundation] Implemented chdir

Foundation implementation isn't better than POSIX one, and we need to change interface slightly from what it is in either case:
e1e7852 [Foundation] Implemented getcwd

Foundation implementation doesn't handle all cases we do (missing cwd).
b5a4a85 [Foundation] Implemented symlink, c8bea50 [Foundation] Implemented rename

We may eventually have a need for using the ...at POSIX facilities directly (which Foundation doesn't expose), so I'd rather keep the implementation which does that in our own code.
c56dca2 [Foundation] Implemented mkdtemp

We need to change the current implementation, but I prefer to continue to use mkdtemp at least versus reimplementing it, so I will rework this part later.

Bouke changed the title ~~Sr 1005 foundation~~ SR-1005: Replace POSIX module by Foundation Apr 26, 2016

Bouke force-pushed the sr-1005-foundation branch from bfe7627 to f082161 Compare May 2, 2016 07:44

Bouke force-pushed the sr-1005-foundation branch 2 times, most recently from ee28942 to ca6fd84 Compare May 6, 2016 21:34

Bouke force-pushed the sr-1005-foundation branch 4 times, most recently from c10ef55 to 9eed36c Compare May 9, 2016 08:05

ddunbar reviewed May 9, 2016
View reviewed changes

Bouke force-pushed the sr-1005-foundation branch from 9eed36c to 146cc04 Compare May 9, 2016 11:29

Bouke added 3 commits May 11, 2016 13:19

[Foundation] Implemented POSIX.realpath

3c673df

[Foundation] Implemented unlink / rmdir / rmtree

f5728c6

[Foundation] Implemented mkdtemp

c56dca2

Bouke added 8 commits May 11, 2016 14:23

[POSIX] Removed unused functions

3f19129

[Foundation] Implemented symlink

b5a4a85

Dropped relativeTo argument as it wasn't used.

[Foundation] Implemented mkdir

07f50bf

[Foundation] Implemented chdir

24f2878

[Foundation] Implemented getcwd

e1e7852

[Foundation] Resolved performance regression

a1be841

The rmtree is not needed as directories are already removed recursively.

[Foundation] Implemented rename

c8bea50

[POSIX] Removed unused SystemError cases

fed2081

Bouke force-pushed the sr-1005-foundation branch from 0a4161f to fed2081 Compare May 11, 2016 12:23

ddunbar mentioned this pull request May 19, 2016

Update file utilities #355

Merged

ddunbar closed this May 19, 2016

Bouke mentioned this pull request Apr 26, 2016

[SR-1005] SwiftPM port to Foundation #5308

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SR-1005: Replace POSIX module by Foundation #284

SR-1005: Replace POSIX module by Foundation #284

Bouke commented Apr 26, 2016 •

edited

Loading

ddunbar commented Apr 26, 2016

Bouke commented Apr 26, 2016

ddunbar commented Apr 26, 2016

Bouke commented Apr 27, 2016 •

edited

Loading

mxcl commented Apr 29, 2016

Bouke commented Apr 30, 2016 •

edited

Loading

Bouke commented May 2, 2016 •

edited

Loading

Bouke commented May 6, 2016

Bouke commented May 7, 2016

ddunbar commented May 9, 2016

ddunbar commented May 9, 2016

ddunbar May 9, 2016

ddunbar commented May 9, 2016

Bouke commented May 9, 2016

ddunbar commented May 9, 2016

Bouke commented May 10, 2016

ddunbar commented May 10, 2016

aciidgh commented May 11, 2016

Bouke commented May 11, 2016

aciidgh commented May 11, 2016

ddunbar commented May 19, 2016

ddunbar commented May 19, 2016

SR-1005: Replace POSIX module by Foundation #284

SR-1005: Replace POSIX module by Foundation #284

Conversation

Bouke commented Apr 26, 2016 • edited Loading

ddunbar commented Apr 26, 2016

Bouke commented Apr 26, 2016

ddunbar commented Apr 26, 2016

Bouke commented Apr 27, 2016 • edited Loading

mxcl commented Apr 29, 2016

Bouke commented Apr 30, 2016 • edited Loading

Bouke commented May 2, 2016 • edited Loading

Bouke commented May 6, 2016

Bouke commented May 7, 2016

ddunbar commented May 9, 2016

ddunbar commented May 9, 2016

ddunbar May 9, 2016

Choose a reason for hiding this comment

ddunbar commented May 9, 2016

Bouke commented May 9, 2016

ddunbar commented May 9, 2016

Bouke commented May 10, 2016

ddunbar commented May 10, 2016

aciidgh commented May 11, 2016

Bouke commented May 11, 2016

aciidgh commented May 11, 2016

ddunbar commented May 19, 2016

ddunbar commented May 19, 2016

Bouke commented Apr 26, 2016 •

edited

Loading

Bouke commented Apr 27, 2016 •

edited

Loading

Bouke commented Apr 30, 2016 •

edited

Loading

Bouke commented May 2, 2016 •

edited

Loading