-
Notifications
You must be signed in to change notification settings - Fork 1.2k
On Windows, use GetTempPath2 for Filename.get_temp_dir_name
#13435
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
Conversation
stdlib/filename.mli
Outdated
| Under Windows, it uses [GetTempPath2] (since Windows 10 21H1), or | ||
| [GetTempPath], and returns: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| Under Windows, it uses [GetTempPath2] (since Windows 10 21H1), or | |
| [GetTempPath], and returns: | |
| Under Windows: |
(how it is implemented is an implementation detail)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added it because the standard libraries of Rust and Golang documented it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't do this anywhere else, so it would be a bit unusual to add it here.
stdlib/filename.ml
Outdated
|
|
||
| let temp_dir_name = | ||
| try Sys.getenv "TMPDIR" with Not_found -> "/tmp" | ||
| let temp_dir_name = temp_dir_name () |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you say a few words about this (seemingly unrelated) change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I needed to move the Windows code to C. It wasn't clear what to return from caml_sys_temp_dir_name for Unix, and as I needed to add an external ... to filename.ml, it seemed more appropriate to move all the code to the C side, including for Unix, rather than adding a dummy symbol for Unix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you convinced by this part of the change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems fine, yes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm slightly less convinced (just on the idea of unintended consequences) - for #12624, we decided it was OK to have "asymmetric" primitives for this kind of thing. That feels OK here, too - so just have the Unix side of the primitive return an empty string and simply not use it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so just have the Unix side of the primitive return an empty string and simply not use it
Applied, thanks!
GetTempPath2 for Filename.get_temp_dir_nameGetTempPath2 for Filename.get_temp_dir_name
467452a to
f19971f
Compare
nojb
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
A second approval is necessary I think. @dra27: could you take a look? Thanks!
dra27
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely for this change, thanks, but it would be good to be motivating it from the security standpoint (what other languages are doing is reassuring, but it's not a reason in and of itself). I'm puzzled by the way InitOnceExecuteOnce is being used - I thought the point of it was to eliminate static variables where the initialisation state was unclear, but I may well be missing a subtlety (cf. the example on MSDN where the point is that there is no static HANDLE value)
stdlib/filename.ml
Outdated
|
|
||
| let temp_dir_name = | ||
| try Sys.getenv "TMPDIR" with Not_found -> "/tmp" | ||
| let temp_dir_name = temp_dir_name () |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm slightly less convinced (just on the idea of unintended consequences) - for #12624, we decided it was OK to have "asymmetric" primitives for this kind of thing. That feels OK here, too - so just have the Unix side of the primitive return an empty string and simply not use it?
50ee123 to
9b794bd
Compare
|
Thanks for the reviews @dra27, @nojb! The code is more simple now. You seem to have divergent opinions on the documentation. I'm more inclined to just mention Note that there is also a call to |
9e4f336 to
4c14a94
Compare
|
I think the CI failure is transient, I cannot reproduce it locally. |
Co-authored-by: David Allsopp <david.allsopp@metastack.com>
4c14a94 to
2a6a3e1
Compare
|
Thanks, @MisterDA! |
We can use the WinAPI to get the temporary directory. It's what Rust and Golang do.
A caveat is that
GetTempPath2is only available since Windows 10 21H1. As older Windows 10 versions are still supported by Microsoft, we need to fallback toGetTempPathifGetTempPath2isn't found.I use a one-time initialization object (available since Vista) to avoid races when initializing the function pointer to either
GetTempPath2orGetTempPath.