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.

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

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
+51 −2
Open

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

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
22500b8
[wasm] Make `FileManager.createFile()` work on WASI
kkebo Oct 19, 2024
77feb1c
[wasm] Make `Data.WritingOptions.atomic` unavailable on WASI
kkebo Oct 24, 2024
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
3 changes: 3 additions & 0 deletions Sources/FoundationEssentials/FileManager/FileManager+Files.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 = []
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 •
edited
Loading

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