[Attempt 3] Implement FileManager.url{s,}(for:…) and NSSearchPathForDirectoriesInDomains. #1600
Conversation
|
@swift-ci please test |
|
cc @rudkx @shahmishal I'm attempting again. I'm trying to figure out why the buildbot found that issue but both local Ubuntu 16.10 and Mac builds didn't find this. |
Did you try it with 16.04? Since it's LTS, it's usually more representative of how Ubuntu behaves :) |
|
We can certainly try merging again and see what the effects are. As @felix91gr says, the same release might provide more confidence this will work, but I don't know any of the details of how the releases are configured (or any customizations in our CI), so I don't know how much of a concern that might be. @millenomi If you do merge, can you keep an eye on the bots and revert ASAP if there are issues? |
|
Could we retry merging the PR? Or is there extra work required? |
|
We can retry but I have been unavailable to check on the patch's progress and CI for a little bit as other work priorities have weighted. |
Foundation/FileManager.swift
Outdated
| case user | ||
|
|
||
| init?(_ domainMask: SearchPathDomainMask) { | ||
| if domainMask == .systemDomainMask { |
There was a problem hiding this comment.
- domainMask is an OptionSet afaik, and urls function will return an array with multiple items if multiple masks are passes I think. I’m not sure however.
There was a problem hiding this comment.
No, they're documented to accept a single mask at a time. Weirdness of the Darwin API, but yeah.
There was a problem hiding this comment.
My bad — you're correct, and this patch is wrong. (url… accepts a single mask, but urls… does not.)
Foundation/FileManager.swift
Outdated
| if domainMask == .systemDomainMask { | ||
| self = .system; return | ||
| } | ||
| if domainMask == .localDomainMask { |
There was a problem hiding this comment.
Why not use switch case instead?
There was a problem hiding this comment.
IIRC I tried, Swift complained, and I switched over without investigating.
|
@swift-ci please test |
|
Turns out it is indeed helpful if I remember to ask the bot for testing. |
|
This has gone on long enough. I'm going to hit that merge button, and we'll deal with bugs in future follow-ups. |
This re-reproposes the patch at #1597 that was reverted by #1598. It should fix the issues from the previous two attempts. (Whoops.)