Skip to content
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

Merged
merged 8 commits into from
May 8, 2020

Conversation

jneira
Copy link
Member

@jneira jneira commented May 3, 2020

  • In the process to use hie-bios on ghc i hit a problem when the hie-bios script tried to write in %HIE_BIOS_OUTPUT$: the file was locked by the executable and the script could not write in it
    • As usual in posis systems the lock works in a way the process started by hie-bios can write
  • Now the file es created but it is not opened until the process has used it. It is not deleted auto when it finishes but i thin kit is no a big problem, being a temporal file (but i could add a deleting)
  • I've changed the handling of %HIE_BIOS_OUTPUT$` to honour the possible previous value set by the user: maybe it can be used if users pnly have permissions over concrete directories or they want to keep the compiler args in a file controled by them

@jneira jneira requested a review from fendor May 3, 2020 13:35
@mpickering
Copy link
Collaborator

This will leak bios-output files into TMP. You need to clean up the temporary file after the cradle has finished running.

@jneira
Copy link
Member Author

jneira commented May 4, 2020

@mpickering done! it is clearner now

src/HIE/Bios/Cradle.hs Outdated Show resolved Hide resolved
@jneira jneira requested review from fendor and mpickering May 6, 2020 21:50
hSetBuffering handle LineBuffering
!res <- force <$> hGetContents handle
return res
else return ""
Copy link
Collaborator

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?

Copy link
Member Author

@jneira jneira May 7, 2020

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?

Copy link
Member Author

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:

https://github.com/mpickering/hie-bios/blob/f0abff9c855ea7e6624617df669825f3f62f723b/src/HIE/Bios/Cradle.hs#L489-L490

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

Copy link
Collaborator

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.

Copy link
Member Author

@jneira jneira May 7, 2020

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

src/HIE/Bios/Cradle.hs Outdated Show resolved Hide resolved
src/HIE/Bios/Cradle.hs Outdated Show resolved Hide resolved
@jneira
Copy link
Member Author

jneira commented May 7, 2020

@mpickering @fendor i think it is done, thanks for the review and suggestions.
I tested the last version in my win 10 with the ghc config and it works: it uses the file pointed by HIE_BIOS_OUTPUT and a temp file (like before) if it is not

@mpickering
Copy link
Collaborator

Thanks LGTM. Over to you @fendor

Copy link
Collaborator

@fendor fendor left a 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 :/

@fendor fendor merged commit 501cc33 into haskell:master May 8, 2020
@jneira
Copy link
Member Author

jneira commented May 8, 2020

Thanks!, only a correction to my previous comments for the shake of being precise: with the change from withSystemTempDirectory to withSystemTempFile the behaviour is the same as before the pr and the file is always created.
So the check for the existence of the file had no sense anymore (again like before the pr) and it is well removed.
However, the file can be empty and it would no trigger any error. I've checked it sending the output of hadrian/hie-bios.bat to nul and running hie-bios check ghc\Main.hs to see the temp file was created but compiler options are empty.
That is the reason that stack path call is succesful although it does not write anything to $HIE_BIOS_OUTPUT.

@jneira jneira deleted the avoid-win-lock branch May 9, 2020 16:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants