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

kkebo · 2024-10-19T14:04:34Z

FileManager.createFile() currently doesn't work on WASI because it requires .atomic, it requires creating a temporary file, and it isn't suppported on WASI.

So I have fixed that by removing the .atomic requirement only on WASI.

fixes swiftwasm/swift#5593 `FileManager.createFile()` currently doesn't work on WASI because it requires `.atomic`, it requires creating a temporary file, and it isn't suppported on WASI. So I have fixed that by removing the `.atomic` requirement only on WASI.

parkera · 2024-10-23T22:46:54Z

Sources/FoundationEssentials/FileManager/FileManager+Files.swift

@@ -276,6 +276,9 @@ extension _FileManagerImpl {
            }
            attr?[.protectionKey] = nil
        }
+        #elseif os(WASI)
+        // WASI doesn't support a temporary file, so can't use `.atomic`
+        let opts: Data.WritingOptions = []


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.

@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()

Can we use availability annotations for WASI yet?

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

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

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

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)

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.

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.

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

I'll try it.

@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)

`writeToFileAux`, `createTemporaryFile`, and `createProtectedTemporaryFile` also become unavailable on WASI.

parkera · 2024-10-24T18:31:50Z

@swift-ci test

kkebo · 2024-10-24T19:20:18Z

macOS CI failure looks like a failure to download the toolchain. If so, could you please retry it?

parkera · 2024-10-25T15:54:18Z

@swift-ci test

kkebo · 2024-12-06T10:21:56Z

Is there anything left I should do?

parkera · 2024-12-10T01:04:17Z

Sources/FoundationEssentials/Data/Data+Writing.swift

@@ -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)


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

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

@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 {

kkebo mentioned this pull request Oct 19, 2024

test(wasi): remove a workaround for FileManager.createFile() kkebo/swift-format#115

Draft

parkera reviewed Oct 23, 2024

View reviewed changes

[wasm] Make Data.WritingOptions.atomic unavailable on WASI

77feb1c

`writeToFileAux`, `createTemporaryFile`, and `createProtectedTemporaryFile` also become unavailable on WASI.

parkera requested a review from jmschonfeld October 24, 2024 18:30

parkera reviewed Dec 10, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

kkebo commented Oct 19, 2024

parkera Oct 23, 2024

kkebo Oct 24, 2024

parkera Oct 24, 2024

parkera Oct 24, 2024

kkebo Oct 24, 2024

parkera Oct 24, 2024

parkera Oct 24, 2024

parkera Oct 24, 2024

kkebo Oct 24, 2024

kkebo Oct 24, 2024 •

edited

Loading

parkera commented Oct 24, 2024

kkebo commented Oct 24, 2024

parkera commented Oct 25, 2024

kkebo commented Dec 6, 2024

parkera Dec 10, 2024

parkera Dec 10, 2024

kkebo Dec 10, 2024

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

Are you sure you want to change the base?

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

Conversation

kkebo commented Oct 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kkebo Oct 24, 2024 • edited Loading

Choose a reason for hiding this comment

parkera commented Oct 24, 2024

kkebo commented Oct 24, 2024

parkera commented Oct 25, 2024

kkebo commented Dec 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

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

kkebo Oct 24, 2024 •

edited

Loading