search

apple / swift-corelibs-foundation

The Foundation Project, providing core utilities, internationalization, and OS independence
swift.org
Apache License 2.0
5.23k stars 1.12k forks source link

[wasm] Do not set permissions in `Data.write` #4976

Closed kkebo closed 3 weeks ago

kkebo commented 3 weeks ago

This fixes https://github.com/swiftwasm/swift/issues/5584.

This PR does not affect platforms other than WASI.

kateinoigakukun commented 3 weeks ago

Good catch! Don't we need to fix _setAttributes too?

kkebo commented 3 weeks ago

@kateinoigakukun

Don't we need to fix _setAttributes too?

I'm sorry, but I don't understand what you mean by "fix _setAttributes".

I changed permissions so that the condition of the if statement are always not satisfied instead of enclosing the following lines with #if !os(WASI).

(If you have a better solution, I'm ok to close this PR.)

kateinoigakukun commented 3 weeks ago

@kkebo Sorry, I meant it might be better to ignore the permission attribute within FileManager.setAttributes. Currently all users of the API also need to exclude FileAttributeKey.posixPermissions entry from the passing attributes manually as we do in this PR but it's just annoying for most of the use cases...

kkebo commented 3 weeks ago

@kateinoigakukun Thank you. That makes sense and it can solve many problems with less code. On the other hand, since it is true that the posixPermissions key is not supported on WASI, I feel that we should throw an error and notify programmers, and also programmers should not try to set posixPermissions in the first place.

kateinoigakukun commented 3 weeks ago

Okay, given that the API is not so widely used directly, asking users to change their code might be practically reasonable 👍

kateinoigakukun commented 3 weeks ago

@swift-ci test

parkera commented 3 weeks ago

We're going to lose this diff once we delete this file in the package branch. Please open a corresponding patch for https://github.com/apple/swift-foundation.

parkera commented 3 weeks ago

Actually, this one may persist because this change was only made to the class type NSData and not the struct - was that intentional?

kkebo commented 3 weeks ago

@parkera

Actually, this one may persist because this change was only made to the class type NSData and not the struct - was that intentional?

At least in swift-corelibs-foundation, the Data.write method calls the NSData.write method and does not have its own implementation.

parkera commented 3 weeks ago

This will soon no longer be the case - see here: https://github.com/apple/swift-foundation/blob/main/Sources/FoundationEssentials/Data/Data%2BWriting.swift

kkebo commented 3 weeks ago

Yes, I know swift-foundation's Data.write.

Besides, I have read your comment before, and I know we will eventually use the implementation of swift-foundation instead of this repository, but I don't know how much effort we need to make swift-foundation compatible with Wasm.