[wasm] Make FileManager.createFile() work on WASI
#992
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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
reviewed
Oct 23, 2024
| @@ -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 = [] | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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()There was a problem hiding this comment.
Can we use availability annotations for WASI yet?
@available(wasi?, unavailable, message: "atomic writing is unavailable in WASI")
There was a problem hiding this comment.
I would also be curious to learn more about why we can't support the option on WASI in the first place. No mkstemp?
There was a problem hiding this comment.
Can we use availability annotations for WASI yet?
No, we can't yet. So I use #if !os(WASI) instead.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Thank you for your suggestion! (and thank you @stephentyrone)
I'll try it.
There was a problem hiding this comment.
@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.
|
@swift-ci test |
|
macOS CI failure looks like a failure to download the toolchain. If so, could you please retry it? |
|
@swift-ci test |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
.atomicrequirement only on WASI.