NIOFileSystem: Try ${TMPDIR} first for temporary directory
#3067
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
Motivation:
This PR aligns what temp directory NIOFileSystem will return first:
- First try `${TMPDIR}` as it's an expressed user preference
- On Darwin, try `_CS_DARWIN_USER_TMP_DIR` next. If it's set, return that.
- Finally, fall back on `/tmp` on Darwin and Linux, and `/data/local/tmp` on Android.
Closes apple#2861.
Modifications:
- Reworks `NIOFileSystem.temporaryDirectory`.
Result:
- NIOFileSystem will now try `${TMPDIR}` first for the temporary directory.
Caveats:
- We might want to align how Swift-NIO and FoundationEssentials resolve temp directory,
and [FoundationEssentials have a different approach](https://github.com/swiftlang/swift-foundation/blob/9d57f36de757b3d5e3a2f7ffcf27aaec3033509f/Sources/FoundationEssentials/String/String%2BPath.swift#L484-L533). But, I agree with [this comment](apple#2861 (comment)), it feels like TMPDIR is an expected default.
|
Thanks for this patch! I've asked @glbrntt to review, as he's most familiar with this section of the code. |
|
Thanks, I prefer this for Android and currently patch this to use Foundation's |
glbrntt
requested changes
Jan 20, 2025
Comment on lines
+1685
to
+1695
| let originalTMPDIR = getenv("TMPDIR") | ||
| defer { | ||
| if let originalTMPDIR { | ||
| setenv("TMPDIR", String(cString: originalTMPDIR), 1) | ||
| } else { | ||
| unsetenv("TMPDIR") | ||
| } | ||
| } | ||
|
|
||
| let customTmpDir = "/custom/tmp/dir" | ||
| setenv("TMPDIR", customTmpDir, 1) |
There was a problem hiding this comment.
I'm a bit hesitant to mess around with TMPDIR like this in case other tests running in parallel are relying on it.
II think my preference here would be to check if TMPDIR is set and then check fs.temporaryDirectory if it was set.
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.
Motivation:
This PR aligns what temp directory NIOFileSystem will return first:
${TMPDIR}as it's an expressed user preference_CS_DARWIN_USER_TMP_DIRnext. If it's set, return that./tmpon Darwin and Linux, and/data/local/tmpon Android.Closes #2861.
Modifications:
NIOFileSystem.temporaryDirectory.Result:
${TMPDIR}first for the temporary directory.Caveats: