Eagerly close tempfile to fix #1082 #1087
Conversation
src/tool.rs
Outdated
| .write_all(include_bytes!("detect_compiler_family.c"))?; | ||
| // Close the file handle *now*, otherwise the compiler may fail to open it on Windows | ||
| // (#1082). The file stays on disk and its path remains valid until `tmp` is dropped. | ||
| tmp.close(); |
There was a problem hiding this comment.
Let's also do a tmp.file().flush()?;
There was a problem hiding this comment.
That would call the Write::flush method, which for File is a no-op on all mainstream platforms including all that cc supports right now: Windows, Unix, and wasi. If you intend the call as precaution against adding a BufWriter or something in the future, sure, I can add it. However, if you want to surface I/O errors that would otherwise be detected while closing the file and ignored by dropping, we'll need File::sync_all as discussed in File's documentation. Which option do you prefer?
There was a problem hiding this comment.
Thanks for investigating this, I'd like file.flush()?; and file.sync_data()?; to be called.
I don't think we need file.sync_metadata()?;, since it's a tempfile.
src/tempfile.rs
Outdated
| &self.path | ||
| } | ||
|
|
||
| pub(super) fn file(&self) -> &File { |
There was a problem hiding this comment.
Let's change this method, to fn take_file() -> Option<File>, so we don't need the close() method
|
Thank you! Just two suggestions I'd want you to change |
2 participants
Fixed #1082