-
Notifications
You must be signed in to change notification settings - Fork 62
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
Avoid error in windows due to temp file being locked #175
Conversation
This will leak bios-output files into TMP. You need to clean up the temporary file after the cradle has finished running. |
@mpickering done! it is clearner now |
src/HIE/Bios/Cradle.hs
Outdated
hSetBuffering handle LineBuffering | ||
!res <- force <$> hGetContents handle | ||
return res | ||
else return "" |
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.
This should be an error rather than returning empty string?
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.
well, it seems there are test cases that actually dont write anything in the HIE_BIOS_OUTPUT (they failed before ad2a675 cause they dont even create the file to write in) so i assumed that it is a known-allowed case (see https://travis-ci.org/github/mpickering/hie-bios/jobs/683972142#L764 f.e.)
maybe is it the other way around and tests were wrong?
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.
well it turns out that readProcessWithOutputFile
is used to make calls (well only one of them actually does it) that are not related with the args and consume the stdout/stderr:
so it seems it is fine in some cases (one?) to ignore the fact that the call has not written anything to HIE_BIOS_OUTPUT
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 would really rather not return empty string here as imagine this situation.
- User consults the cradle, the file doesn't exist for some unknown reason
- The cradle reports "" as the output
- ghcide starts a session with no options
- User sees error about import of module failing
Now to get from the first step to the last step is not trivial to realise unless you are familar with how all the parts work.
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.
ok, i've removed the file existence check as you suggested so it will crash early
@mpickering @fendor i think it is done, thanks for the review and suggestions. |
Thanks LGTM. Over to you @fendor |
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.
Great thank you! Sorry it took me so long to review :/
Thanks!, only a correction to my previous comments for the shake of being precise: with the change from |
%HIE_BIOS_OUTPUT$
: the file was locked by the executable and the script could not write in it