Skip to content

Implement FilePath.getCurrentWorkingDirectory() #71

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 3 commits into from
Closed

Implement FilePath.getCurrentWorkingDirectory() #71

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
cc9e58a
Implement FilePath.getCurrentWorkingDirectory()
GeorgeLyon Nov 13, 2021
35dfffd
Make process-global nature of working directory more apparent
GeorgeLyon Nov 13, 2021
3aef396
Change name again
GeorgeLyon Nov 15, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions Sources/System/FilePath/FilePath.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,18 @@ public struct FilePath {

// @available(macOS 10.16, iOS 14.0, watchOS 7.0, tvOS 14.0, *)
extension FilePath {

/// Returns the current working directory of this process.
///
/// - Warning: This value is global and care should be taken to make sure it does not unexpectedly change.
public static func getCurrentWorkingDirectoryForProcess() throws -> FilePath {
Copy link
Member

@lorentey lorentey Nov 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should lose the get prefix -- per the Swift API design guidelines, functions without side effects should read as noun phrases.

I know you specifically added this in response to discussions, but I would like to also remove the ForProcess suffix. There are two reasons for this:

  • First, I don't think it makes sense to try documenting the precise semantics of these functions through their naming -- it seems futile to me, and it leads to an unwieldy naming scheme.

  • Far, far more importantly, while the default semantics of these syscalls have been pretty stable, there is no reason an OS cannot switch to different behavior in certain circumstances. For instance, on Linux and macOS at least we have non-portable ways to set up a thread-local working directory.

Suggested change
public static func getCurrentWorkingDirectoryForProcess() throws -> FilePath {
public static func currentWorkingDirectory() throws -> FilePath {

guard let cwd = system_getcwd(nil, 0) else {
Copy link
Member

@lorentey lorentey Nov 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will allocate twice -- wouldn't it be better if we supplied this with an appropriately sized FilePath buffer and only resize it if we guessed wrong?

If desired, we can also eliminate allocations altogether by adding a variant that takes an inout FilePath. (I don't think we need that.)

Copy link
Contributor Author

@GeorgeLyon GeorgeLyon Nov 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might be better but given all the discussion about how this shouldn't be used often, I preferred the simpler variant rather than adding an infrequently taken second codepath.

Copy link
Contributor

@milseman milseman Nov 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FilePath used to have a with-style internal init that used max-path-length, but it turns out there are multiple notions of such a thing and it's probably too complicated as-is (and wastes space). I agree that it doesn't matter for now; in the future we'd like to do it with something like an init on FilePath, which can make the space/speed tradeoffs.

throw Errno.current
}
defer { system_free(cwd) }
return FilePath(platformString: cwd)
}

Copy link
Contributor

@milseman milseman Nov 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Way back when I was thinking this would be hosted on some kind of current process type, but I think this approach makes more sense. It's also available for typename elision IIUC:

func foo(_: FilePath)
let x = foo(.getCurrentWorkingDirectory())

I feel like making this a function rather than a throwing var makes sense (ignoring tools version concerns) as it will make a syscall.

/// The length of the file path, excluding the null terminator.
public var length: Int { _storage.length }
}
Expand Down
21 changes: 21 additions & 0 deletions Sources/System/Internals/Syscalls.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,3 +123,24 @@ internal func system_pipe(_ fds: UnsafeMutablePointer<Int32>) -> CInt {
return pipe(fds)
}
#endif

#if !os(Windows)
internal func system_getcwd(_ buf: UnsafeMutablePointer<CInterop.PlatformChar>?, _ size: size_t) -> UnsafeMutablePointer<CInterop.PlatformChar>? {
#if ENABLE_MOCKING
if mockingEnabled {
/// I'm not really sure how to mock this since `buf` is passed in uninitialized and it needs to return something
fatalError()
}
#endif
return getcwd(buf, size)
}
#endif

#if !os(Windows)
internal func system_free(_ ptr: UnsafeMutableRawPointer?) {
#if ENABLE_MOCKING
if mockingEnabled { _ = _mock(ptr) }
#endif
free(ptr)
}
#endif
6 changes: 6 additions & 0 deletions Tests/SystemTests/FilePathTests/FilePathTest.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,5 +72,11 @@ final class FilePathTest: XCTestCase {
}
}
}

func testCurrentWorkingDirectory() {
XCTAssertEqual(
try FilePath.getCurrentWorkingDirectoryForProcess().string,
FileManager.default.currentDirectoryPath)
}
}