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

FileManager.default.createFile() doesn't work #5593

Open
kkebo opened this issue Oct 10, 2024 · 6 comments · May be fixed by swiftlang/swift-foundation#992
Open

FileManager.default.createFile() doesn't work #5593

kkebo opened this issue Oct 10, 2024 · 6 comments · May be fixed by swiftlang/swift-foundation#992

Comments

@kkebo
Copy link

kkebo commented Oct 10, 2024

Description

FileManager.default.createFile() doesn't work.

Steps to reproduce

  1. mkdir foo
  2. cd foo
  3. swift package init --type executable
  4. Edit Sources/main.swift
    import Foundation
    
    print(FileManager.default.createFile(atPath: "/tmp/foo", contents: Data()))
  5. swift build --swift-sdk <Swift SDK for Wasm>
  6. wasmtime run --dir /tmp .build/debug/foo.wasm

Expected behavior

$ wasmtime run --dir /tmp .build/debug/foo.wasm
true

Actual behavior

$ wasmtime run --dir /tmp .build/debug/foo.wasm
false

Environment

$ uname -a
Linux Raspberry-beetle 6.6.51+rpt-rpi-2712 #1 SMP PREEMPT Debian 1:6.6.51-1+rpt2 (2024-10-01) aarch64 GNU/Linux
$ cat /etc/os-release
PRETTY_NAME="Debian GNU/Linux 12 (bookworm)"
NAME="Debian GNU/Linux"
VERSION_ID="12"
VERSION="12 (bookworm)"
VERSION_CODENAME=bookworm
ID=debian
HOME_URL="https://www.debian.org/"
SUPPORT_URL="https://www.debian.org/support"
BUG_REPORT_URL="https://bugs.debian.org/"
$ swift --version
Swift version 6.0.2-dev (LLVM 43d73cb0cc589f7, Swift aaa632cea622394)
Target: aarch64-unknown-linux-gnu
$ swift sdk list
6.0-SNAPSHOT-2024-09-18-a-wasm32-unknown-wasi
$ wasmtime -V
wasmtime 25.0.1 (b4cb894c9 2024-09-24)

Supplement

swift-foundation's createFile() implementation forces to use the option .atomic.

So we can see the detailed error by running try Data().write(to: URL(filePath: "/tmp/foo"), options: .atomic).

Swift/ErrorType.swift:253: Fatal error: Error raised at top level: FoundationEssentials.CocoaError(code: FoundationEssentials.CocoaError.Code(rawValue: 3328), userInfo: ["NSUserStringVariant": "Folder"])
Error: failed to run main module `.build/debug/hoge.wasm`

Caused by:
    0: failed to invoke command default
    1: error while executing at wasm backtrace:
           0: 0x6c1b71 - <unknown>!$ss17_assertionFailure__4file4line5flagss5NeverOs12StaticStringV_SSAHSus6UInt32VtF
           1: 0x76a8da - <unknown>!swift_errorInMain
           2: 0x2c755 - <unknown>!main
           3: 0xc2f036 - <unknown>!__main_void
           4: 0x2c2b9 - <unknown>!_start
       note: using the `WASMTIME_BACKTRACE_DETAILS=1` environment variable may show more debugging information
    2: wasm trap: wasm `unreachable` instruction executed

If options contain .atomic, createFile() will call writeToFileAux(), and writeToFileAux() will call createProtectedTemporaryFile() and createTemporaryFile(). However, SwiftWasm doesn't support createTemporaryFile().

How about stopping adding .atomic in createFile() if os(WASI)?

@kkebo
Copy link
Author

kkebo commented Oct 14, 2024

Another solution is not to call createTemporaryFile() from createProtectedTemporaryFile() if os(WASI). createTemporaryFile() is only used in createProtectedTemporaryFile(), so I think we don't necessary have to fix createTemporaryFile() if we fix on the createProtectedTemporaryFile() side.

diff --git a/Sources/FoundationEssentials/Data/Data+Writing.swift b/Sources/FoundationEssentials/Data/Data+Writing.swift
index 7c23e1e..29815b0 100644
--- a/Sources/FoundationEssentials/Data/Data+Writing.swift
+++ b/Sources/FoundationEssentials/Data/Data+Writing.swift
@@ -237,9 +237,28 @@ private func createProtectedTemporaryFile(at destinationPath: String, inPath: Pa
     }
 #endif
     
+#if os(WASI)
+    let url = URL(fileURLWithPath: destinationPath, isDirectory: false)
+    let temporaryDirectoryPath = try FileManager.default.url(for: .itemReplacementDirectory, in: .userDomainMask, appropriateFor: url, create: true).path(percentEncoded: false)
+    let auxFile = temporaryDirectoryPath.appendingPathComponent(destinationPath.lastPathcomponent)
+    return try auxFile.withFileSystemRepresentation { auxFileFileSystemRep in
+        guard let auxFileFileSystemRep else {
+            throw CocoaError(.fileWriteInvalidFileName)
+        }
+        let fd = openFileDescriptorProtected(path: auxFileFileSystemRep, flags: O_CREAT | O_EXCL | O_RDWR, options: options)
+        if fd >= 0 {
+            return (fd, auxFile, temporaryDirectoryPath)
+        } else {
+            let savedErrno = errno
+            cleanupTemporaryDirectory(at: temporaryDirectoryPath)
+            throw CocoaError.errorWithFilePath(inPath, errno: savedErrno, reading: false, variant: variant)
+        }
+    }
+#else
     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
 }
 
 private func write(buffer: UnsafeRawBufferPointer, toFileDescriptor fd: Int32, path: PathOrURL, parentProgress: Progress?) throws {

https://github.com/swiftlang/swift-foundation/blob/3f9171072dc842c04c77ed2fa578d1b0cd123a04/Sources/FoundationEssentials/Data/Data%2BWriting.swift#L240-L242

@kateinoigakukun
Copy link
Member

I think .itemReplacementDirectory is not always writable on WASI, so I prefer the way of stopping adding .atomic.

@kkebo
Copy link
Author

kkebo commented Oct 14, 2024

That makes sense, and I agree with the use of non-atomic writing for createFile().

On the other hand, though this is off the main topic of the issue, I think it would be worthwhile to support the creation of temporary files using .itemReplacementDirectory for those who want to use atomic writing (e.g. write(to: ..., options: .atomic)).

@kateinoigakukun
Copy link
Member

How about allowing atomic file writing when TMPDIR environment variable is explicitly defined?

@kkebo
Copy link
Author

kkebo commented Oct 15, 2024

Thank you for your consideration. However, this comment has changed my mind, and I'm beginning to think that relying on $TMPDIR is not a good idea.

@kkebo
Copy link
Author

kkebo commented Oct 19, 2024

I created another issue #5594 for discussing atomic file writing.

kkebo added a commit to kkebo/swift-foundation that referenced this issue Oct 19, 2024
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants