Skip to content
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.

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

[wasm] Make FileManager.createFile() work on WASI #992

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 4 additions & 0 deletions Benchmarks/Benchmarks/DataIO/BenchmarkDataIO.swift
Original file line number Diff line number Diff line change
Expand Up @@ -88,9 +88,11 @@ let benchmarks = {
try data.write(to: testPath)
}

#if !os(WASI) // atomic writing is unavailable on WASI
Benchmark("write-regularFile-atomic", configuration: .cleanupTestPathConfig) { benchmark in
try data.write(to: testPath, options: .atomic)
}
#endif

Benchmark("write-regularFile-alreadyExists",
configuration: .init(
Expand All @@ -103,6 +105,7 @@ let benchmarks = {
try? data.write(to: testPath)
}

#if !os(WASI) // atomic writing is unavailable on WASI
Benchmark("write-regularFile-alreadyExists-atomic",
configuration: .init(
setup: {
Expand All @@ -113,6 +116,7 @@ let benchmarks = {
) { benchmark in
try? data.write(to: testPath, options: .atomic)
}
#endif

Benchmark("read-regularFile",
configuration: .init(
Expand Down
27 changes: 25 additions & 2 deletions Sources/FoundationEssentials/Data/Data+Writing.swift
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,9 @@ private func cleanupTemporaryDirectory(at inPath: String?) {
}

/// Caller is responsible for calling `close` on the `Int32` file descriptor.
#if os(WASI)
@available(*, unavailable, message: "WASI does not have temporary directories")
#endif
private func createTemporaryFile(at destinationPath: String, inPath: PathOrURL, prefix: String, options: Data.WritingOptions, variant: String? = nil) throws -> (Int32, String) {
#if os(WASI)
// WASI does not have temp directories
Expand Down Expand Up @@ -200,7 +203,14 @@ private func createTemporaryFile(at destinationPath: String, inPath: PathOrURL,

/// Returns `(file descriptor, temporary file path, temporary directory path)`
/// Caller is responsible for calling `close` on the `Int32` file descriptor and calling `cleanupTemporaryDirectory` on the temporary directory path. The temporary directory path may be nil, if it does not need to be cleaned up.
#if os(WASI)
@available(*, unavailable, message: "WASI does not have temporary directories")
#endif
private func createProtectedTemporaryFile(at destinationPath: String, inPath: PathOrURL, options: Data.WritingOptions, variant: String? = nil) throws -> (Int32, String, String?) {
#if os(WASI)
// WASI does not have temp directories
throw CocoaError(.featureUnsupported)
#else
#if FOUNDATION_FRAMEWORK
if _foundation_sandbox_check(getpid(), nil) != 0 {
// Convert the path back into a string
Expand Down Expand Up @@ -240,6 +250,7 @@ private func createProtectedTemporaryFile(at destinationPath: String, inPath: Pa
let temporaryDirectoryPath = destinationPath.deletingLastPathComponent()
let (fd, auxFile) = try createTemporaryFile(at: temporaryDirectoryPath, inPath: inPath, prefix: ".dat.nosync", options: options, variant: variant)
return (fd, auxFile, nil)
#endif // os(WASI)
}

private func write(buffer: UnsafeRawBufferPointer, toFileDescriptor fd: Int32, path: PathOrURL, parentProgress: Progress?) throws {
Expand Down Expand Up @@ -314,15 +325,26 @@ internal func writeToFile(path inPath: PathOrURL, data: Data, options: Data.Writ
}

internal func writeToFile(path inPath: PathOrURL, buffer: UnsafeRawBufferPointer, options: Data.WritingOptions, attributes: [String : Data] = [:], reportProgress: Bool = false) throws {
#if os(WASI) // `.atomic` is unavailable on WASI
try writeToFileNoAux(path: inPath, buffer: buffer, options: options, attributes: attributes, reportProgress: reportProgress)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this also throw like the public entry point, in case someone adds a new call to it that circumvents the outer one?

Copy link
Contributor

Choose a reason for hiding this comment

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

Or, put it inside the helper function writeToFileAux instead and leave this one without a diff?

Copy link
Author

Choose a reason for hiding this comment

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

@parkera Thank you for reviewing. I tried my best to understand your comments, but I may not be able to imagine the actual examples you are concerned about.

Should this also throw like the public entry point, in case someone adds a new call to it that circumvents the outer one?

I don't think such a case will happen, or we can notice it immediately if it happens.

If someone adds a new call like try writeToFile(path: ..., buffer: ..., options: .atomic), then it will cause a compile error when compiling swift-foundation for WASI, because .atomic is unavailable if os(WASI).

Example:

/home/kebo/swift-foundation/Sources/FoundationEssentials/Data/Data+Writing.swift:329:69: error: 'atomic' is unavailable: atomic writing is unavailable in WASI because temporary files are not supported
327 | func foo() throws {
328 |     try Data().withUnsafeBytes { buf in
329 |         try writeToFile(path: .path("/foo"), buffer: buf, options: .atomic)
    |                                                                     `- error: 'atomic' is unavailable: atomic writing is unavailable in WASI because temporary files are not supported
330 |     }
331 | }

/home/kebo/swift-foundation/Sources/FoundationEssentials/Data/Data.swift:2063:27: note: 'atomic' has been explicitly marked unavailable here
2061 |         @available(*, unavailable, message: "atomic writing is unavailable in WASI because temporary files are not supported")
2062 | #endif
2063 |         public static let atomic = WritingOptions(rawValue: 1 << 0)
     |                           `- note: 'atomic' has been explicitly marked unavailable here
2064 |         
2065 |         /// An option that attempts to write data to a file and fails with an error if the destination file already exists.

Or, put it inside the helper function writeToFileAux instead and leave this one without a diff?

Since .atomic and writeToFileAux are unavailable in WASI, it is impossible to leave this method without a diff.

/home/kebo/swift-foundation/Sources/FoundationEssentials/Data/Data+Writing.swift:328:26: error: 'atomic' is unavailable: atomic writing is unavailable in WASI because temporary files are not supported
326 | 
327 | internal func writeToFile(path inPath: PathOrURL, buffer: UnsafeRawBufferPointer, options: Data.WritingOptions, attributes: [String : Data] = [:], reportProgress: Bool = false) throws {
328 |     if options.contains(.atomic) {
    |                          `- error: 'atomic' is unavailable: atomic writing is unavailable in WASI because temporary files are not supported
329 |         try writeToFileAux(path: inPath, buffer: buffer, options: options, attributes: attributes, reportProgress: reportProgress)
330 |     } else {

#else
if options.contains(.atomic) {
try writeToFileAux(path: inPath, buffer: buffer, options: options, attributes: attributes, reportProgress: reportProgress)
} else {
try writeToFileNoAux(path: inPath, buffer: buffer, options: options, attributes: attributes, reportProgress: reportProgress)
}
#endif
}

/// Create a new file out of `Data` at a path, using atomic writing.
#if os(WASI)
@available(*, unavailable, message: "atomic writing is unavailable in WASI because temporary files are not supported")
#endif
private func writeToFileAux(path inPath: PathOrURL, buffer: UnsafeRawBufferPointer, options: Data.WritingOptions, attributes: [String : Data], reportProgress: Bool) throws {
#if os(WASI)
// `.atomic` is unavailable on WASI
throw CocoaError(.featureUnsupported)
#else
assert(options.contains(.atomic))

// TODO: Somehow avoid copying back and forth to a String to hold the path
Expand Down Expand Up @@ -495,7 +517,6 @@ private func writeToFileAux(path inPath: PathOrURL, buffer: UnsafeRawBufferPoint

cleanupTemporaryDirectory(at: temporaryDirectoryPath)

#if !os(WASI) // WASI does not support fchmod for now
if let mode {
// Try to change the mode if the path has not changed. Do our best, but don't report an error.
#if FOUNDATION_FRAMEWORK
Expand All @@ -519,16 +540,18 @@ private func writeToFileAux(path inPath: PathOrURL, buffer: UnsafeRawBufferPoint
fchmod(fd, mode)
#endif
}
#endif // os(WASI)
}
}
}
#endif
#endif // os(WASI)
}

/// Create a new file out of `Data` at a path, not using atomic writing.
private func writeToFileNoAux(path inPath: PathOrURL, buffer: UnsafeRawBufferPointer, options: Data.WritingOptions, attributes: [String : Data], reportProgress: Bool) throws {
#if !os(WASI) // `.atomic` is unavailable on WASI
assert(!options.contains(.atomic))
#endif

#if os(Windows)
try inPath.path.withNTPathRepresentation { pwszPath in
Expand Down
5 changes: 5 additions & 0 deletions Sources/FoundationEssentials/Data/Data.swift
Original file line number Diff line number Diff line change
Expand Up @@ -2057,6 +2057,9 @@ public struct Data : Equatable, Hashable, RandomAccessCollection, MutableCollect
public init(rawValue: UInt) { self.rawValue = rawValue }

/// An option to write data to an auxiliary file first and then replace the original file with the auxiliary file when the write completes.
#if os(WASI)
@available(*, unavailable, message: "atomic writing is unavailable in WASI because temporary files are not supported")
#endif
public static let atomic = WritingOptions(rawValue: 1 << 0)

/// An option that attempts to write data to a file and fails with an error if the destination file already exists.
Expand Down Expand Up @@ -2442,9 +2445,11 @@ public struct Data : Equatable, Hashable, RandomAccessCollection, MutableCollect
/// - parameter options: Options for writing the data. Default value is `[]`.
/// - throws: An error in the Cocoa domain, if there is an error writing to the `URL`.
public func write(to url: URL, options: Data.WritingOptions = []) throws {
#if !os(WASI) // `.atomic` is unavailable on WASI
if options.contains(.withoutOverwriting) && options.contains(.atomic) {
fatalError("withoutOverwriting is not supported with atomic")
}
#endif

guard url.isFileURL else {
throw CocoaError(.fileWriteUnsupportedScheme)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -276,6 +276,9 @@ extension _FileManagerImpl {
}
attr?[.protectionKey] = nil
}
#elseif os(WASI)
// `.atomic` is unavailable on WASI
let opts: Data.WritingOptions = []
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems unfortunate to allow a caller to think they are getting the safety of an atomic write and then silently drop it. Maybe it would be better to make the atomic option unavailable in WASI so that the caller has to knowingly not use it.

Copy link
Author

Choose a reason for hiding this comment

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

@parkera Thank you for reviewing the PR. I'm sorry if I don't understanding what you are saying correctly, but do you mean something like this?

diff --git a/Sources/FoundationEssentials/Data/Data.swift b/Sources/FoundationEssentials/Data/Data.swift
index 70df1e8..55a4e33 100644
--- a/Sources/FoundationEssentials/Data/Data.swift
+++ b/Sources/FoundationEssentials/Data/Data.swift
@@ -2056,8 +2056,10 @@ public struct Data : Equatable, Hashable, RandomAccessCollection, MutableCollect
         public let rawValue: UInt
         public init(rawValue: UInt) { self.rawValue = rawValue }
 
+#if !os(WASI)
         /// An option to write data to an auxiliary file first and then replace the original file with the auxiliary file when the write completes.
         public static let atomic = WritingOptions(rawValue: 1 << 0)
+#endif
         
         /// An option that attempts to write data to a file and fails with an error if the destination file already exists.
         public static let withoutOverwriting = WritingOptions(rawValue: 1 << 1)
full diff
diff --git a/Benchmarks/Benchmarks/DataIO/BenchmarkDataIO.swift b/Benchmarks/Benchmarks/DataIO/BenchmarkDataIO.swift
index e660f8a..8bf2cd4 100644
--- a/Benchmarks/Benchmarks/DataIO/BenchmarkDataIO.swift
+++ b/Benchmarks/Benchmarks/DataIO/BenchmarkDataIO.swift
@@ -88,9 +88,11 @@ let benchmarks = {
         try data.write(to: testPath)
     }
     
+#if !os(WASI)
     Benchmark("write-regularFile-atomic", configuration: .cleanupTestPathConfig) { benchmark in
         try data.write(to: testPath, options: .atomic)
     }
+#endif
     
     Benchmark("write-regularFile-alreadyExists",
               configuration: .init(
@@ -103,6 +105,7 @@ let benchmarks = {
         try? data.write(to: testPath)
     }
     
+#if !os(WASI)
     Benchmark("write-regularFile-alreadyExists-atomic",
               configuration: .init(
                 setup: {
@@ -113,6 +116,7 @@ let benchmarks = {
     ) { benchmark in
         try? data.write(to: testPath, options: .atomic)
     }
+#endif
     
     Benchmark("read-regularFile", 
               configuration: .init(
diff --git a/Sources/FoundationEssentials/Data/Data+Writing.swift b/Sources/FoundationEssentials/Data/Data+Writing.swift
index 7c23e1e..ff2c4c9 100644
--- a/Sources/FoundationEssentials/Data/Data+Writing.swift
+++ b/Sources/FoundationEssentials/Data/Data+Writing.swift
@@ -314,13 +314,18 @@ internal func writeToFile(path inPath: PathOrURL, data: Data, options: Data.Writ
 }
 
 internal func writeToFile(path inPath: PathOrURL, buffer: UnsafeRawBufferPointer, options: Data.WritingOptions, attributes: [String : Data] = [:], reportProgress: Bool = false) throws {
+#if os(WASI)
+    try writeToFileNoAux(path: inPath, buffer: buffer, options: options, attributes: attributes, reportProgress: reportProgress)
+#else
     if options.contains(.atomic) {
         try writeToFileAux(path: inPath, buffer: buffer, options: options, attributes: attributes, reportProgress: reportProgress)
     } else {
         try writeToFileNoAux(path: inPath, buffer: buffer, options: options, attributes: attributes, reportProgress: reportProgress)
     }
+#endif
 }
 
+#if !os(WASI)
 /// Create a new file out of `Data` at a path, using atomic writing.
 private func writeToFileAux(path inPath: PathOrURL, buffer: UnsafeRawBufferPointer, options: Data.WritingOptions, attributes: [String : Data], reportProgress: Bool) throws {
     assert(options.contains(.atomic))
@@ -495,7 +500,6 @@ private func writeToFileAux(path inPath: PathOrURL, buffer: UnsafeRawBufferPoint
                 
                 cleanupTemporaryDirectory(at: temporaryDirectoryPath)
                 
-#if !os(WASI) // WASI does not support fchmod for now
                 if let mode {
                     // Try to change the mode if the path has not changed. Do our best, but don't report an error.
 #if FOUNDATION_FRAMEWORK
@@ -519,16 +523,18 @@ private func writeToFileAux(path inPath: PathOrURL, buffer: UnsafeRawBufferPoint
                     fchmod(fd, mode)
 #endif
                 }
-#endif // os(WASI)
             }
         }
     }
 #endif
 }
+#endif  // os(WASI)
 
 /// Create a new file out of `Data` at a path, not using atomic writing.
 private func writeToFileNoAux(path inPath: PathOrURL, buffer: UnsafeRawBufferPointer, options: Data.WritingOptions, attributes: [String : Data], reportProgress: Bool) throws {
+#if !os(WASI)
     assert(!options.contains(.atomic))
+#endif
 
 #if os(Windows)
     try inPath.path.withNTPathRepresentation { pwszPath in
diff --git a/Sources/FoundationEssentials/Data/Data.swift b/Sources/FoundationEssentials/Data/Data.swift
index 70df1e8..55a4e33 100644
--- a/Sources/FoundationEssentials/Data/Data.swift
+++ b/Sources/FoundationEssentials/Data/Data.swift
@@ -2056,8 +2056,10 @@ public struct Data : Equatable, Hashable, RandomAccessCollection, MutableCollect
         public let rawValue: UInt
         public init(rawValue: UInt) { self.rawValue = rawValue }
 
+#if !os(WASI)
         /// An option to write data to an auxiliary file first and then replace the original file with the auxiliary file when the write completes.
         public static let atomic = WritingOptions(rawValue: 1 << 0)
+#endif
         
         /// An option that attempts to write data to a file and fails with an error if the destination file already exists.
         public static let withoutOverwriting = WritingOptions(rawValue: 1 << 1)
@@ -2442,9 +2444,11 @@ public struct Data : Equatable, Hashable, RandomAccessCollection, MutableCollect
     /// - parameter options: Options for writing the data. Default value is `[]`.
     /// - throws: An error in the Cocoa domain, if there is an error writing to the `URL`.
     public func write(to url: URL, options: Data.WritingOptions = []) throws {
+#if !os(WASI)
         if options.contains(.withoutOverwriting) && options.contains(.atomic) {
             fatalError("withoutOverwriting is not supported with atomic")
         }
+#endif
         
         guard url.isFileURL else {
             throw CocoaError(.fileWriteUnsupportedScheme)
diff --git a/Sources/FoundationEssentials/String/String+IO.swift b/Sources/FoundationEssentials/String/String+IO.swift
index 4adc11b..a210e14 100644
--- a/Sources/FoundationEssentials/String/String+IO.swift
+++ b/Sources/FoundationEssentials/String/String+IO.swift
@@ -447,7 +447,12 @@ extension StringProtocol {
             attributes = [:]
         }
 
+#if os(WASI)
+        guard !useAuxiliaryFile else { throw CocoaError(.featureUnsupported) }
+        let options : Data.WritingOptions = []
+#else
         let options : Data.WritingOptions = useAuxiliaryFile ? [.atomic] : []
+#endif
 
         try writeToFile(path: .path(String(path)), data: data, options: options, attributes: attributes, reportProgress: false)
     }
@@ -465,7 +470,12 @@ extension StringProtocol {
             attributes = [:]
         }
 
+#if os(WASI)
+        guard !useAuxiliaryFile else { throw CocoaError(.featureUnsupported) }
+        let options : Data.WritingOptions = []
+#else
         let options : Data.WritingOptions = useAuxiliaryFile ? [.atomic] : []
+#endif
 
         try writeToFile(path: .url(url), data: data, options: options, attributes: attributes, reportProgress: false)
     }
diff --git a/Tests/FoundationEssentialsTests/DataIOTests.swift b/Tests/FoundationEssentialsTests/DataIOTests.swift
index 999ecea..c515cd7 100644
--- a/Tests/FoundationEssentialsTests/DataIOTests.swift
+++ b/Tests/FoundationEssentialsTests/DataIOTests.swift
@@ -91,6 +91,7 @@ class DataIOTests : XCTestCase {
         cleanup(at: url)
     }
 
+#if !os(WASI)
     // Atomic writing is a very different code path
     func test_readWriteAtomic() throws {
         let url = testURL()
@@ -102,6 +103,7 @@ class DataIOTests : XCTestCase {
 
         cleanup(at: url)
     }
+#endif
 
     func test_readWriteMapped() throws {
         let url = testURL()

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use availability annotations for WASI yet?

@available(wasi?, unavailable, message: "atomic writing is unavailable in WASI")

Copy link
Contributor

Choose a reason for hiding this comment

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

I would also be curious to learn more about why we can't support the option on WASI in the first place. No mkstemp?

Copy link
Author

Choose a reason for hiding this comment

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

Can we use availability annotations for WASI yet?

No, we can't yet. So I use #if !os(WASI) instead.

https://github.com/swiftlang/swift/blob/80190ad5397c7cf3bfe07ac21d3aa550679e9afd/include/swift/AST/PlatformKinds.def#L25-L38

Copy link
Contributor

Choose a reason for hiding this comment

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

Or this, if it compiles

#if os(WASI)
@available(*, unavailable, message: "atomic writing is unavailable in WASI")
#endif
/// An option to write data to an auxiliary file first and then replace the original file with the auxiliary file when the write completes.
public static let atomic = WritingOptions(rawValue: 1 << 0)

Copy link
Contributor

Choose a reason for hiding this comment

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

Then we'll have to follow that through all the places we use the atomic option in Foundation itself. But at least it's honest.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's try the workaround above for the short term. But yes, Swift needs to support availability annotations for WASI (and it can have versions, just like macOS or iOS does) - and that is a process we should start in parallel.

Copy link
Author

Choose a reason for hiding this comment

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

Thank you for your suggestion! (and thank you @stephentyrone)

I'll try it.

Copy link
Author

@kkebo kkebo Oct 24, 2024

Choose a reason for hiding this comment

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

@parkera How about this commit? 77feb1c

I found that the following code caused an error. So the diff is a little bit larger.

@available(*, unavailable, message: "foo is unavailable")
func foo() {}

@available(*, unavailable, message: "bar is unavailable")
func bar() {
    foo()  // <-- error: 'foo()' is unavailable: foo is unavailable
}

I confirmed that swift build succeeded.

$ swift --version
Swift version 6.0.3-dev (LLVM 81a2745a5c40b5f, Swift 8f95e04bb5d9d9a)
Target: aarch64-unknown-linux-gnu
$ swift sdk list
6.0-SNAPSHOT-2024-10-10-a-wasm32-unknown-wasi
$ swift build --swift-sdk wasm32-unknown-wasi
...
Build complete! (82.28s)
$ swift build
...
Build complete! (61.87s)

#else
let opts = Data.WritingOptions.atomic
#endif
Expand Down
10 changes: 10 additions & 0 deletions Sources/FoundationEssentials/String/String+IO.swift
Original file line number Diff line number Diff line change
Expand Up @@ -447,7 +447,12 @@ extension StringProtocol {
attributes = [:]
}

#if os(WASI)
guard !useAuxiliaryFile else { throw CocoaError(.featureUnsupported) }
let options : Data.WritingOptions = []
#else
let options : Data.WritingOptions = useAuxiliaryFile ? [.atomic] : []
#endif

try writeToFile(path: .path(String(path)), data: data, options: options, attributes: attributes, reportProgress: false)
}
Expand All @@ -465,7 +470,12 @@ extension StringProtocol {
attributes = [:]
}

#if os(WASI)
guard !useAuxiliaryFile else { throw CocoaError(.featureUnsupported) }
let options : Data.WritingOptions = []
#else
let options : Data.WritingOptions = useAuxiliaryFile ? [.atomic] : []
#endif

try writeToFile(path: .url(url), data: data, options: options, attributes: attributes, reportProgress: false)
}
Expand Down
4 changes: 4 additions & 0 deletions Tests/FoundationEssentialsTests/DataIOTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,9 @@ class DataIOTests : XCTestCase {

// Atomic writing is a very different code path
func test_readWriteAtomic() throws {
#if os(WASI)
try XCTSkip("atomic writing is not supported on WASI")
#else
let url = testURL()
// Perform an atomic write to a file that does not exist
try writeAndVerifyTestData(to: url, writeOptions: [.atomic])
Expand All @@ -101,6 +104,7 @@ class DataIOTests : XCTestCase {
try writeAndVerifyTestData(to: url, writeOptions: [.atomic])

cleanup(at: url)
#endif
}

func test_readWriteMapped() throws {
Expand Down