Skip to content

hsc2hs: Make removeFile more reliable on Windows.#25

Merged
RyanGlScott merged 5 commits intohaskell:masterfrom
Mistuke:gh-77-update-to-new-process
Jun 23, 2019
Merged

hsc2hs: Make removeFile more reliable on Windows.#25
RyanGlScott merged 5 commits intohaskell:masterfrom
Mistuke:gh-77-update-to-new-process

Conversation

@Mistuke
Copy link
Contributor

@Mistuke Mistuke commented Jun 8, 2019

This makes errors such as those in https://gitlab.haskell.org/ghc/ghc/issues/9775#note_203453 a lot rarer.

It does this by using two things:

  1. It uses a process API that on Windows causes waitForProcess to wait for all child processes to complete. This is required because the tools that hsc2hs use call exec which does not behave as you would expect on Windows. The parent will return before the child finishes and so you have a race condition. On other OSes this flag has no effect.

  2. It implements a retry queue which tries to delete a file 6 times if it's a permission error that prevented you from deleting it. This because the temp file could be locked by an AV for scanning. The total wait time is ~3secs.

Copy link
Member

@RyanGlScott RyanGlScott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @Mistuke! I've left some suggestions inline.

Bumping the lower version bounds of process poses an interesting question with respect to hsc2hs's support window, since this patch requires a more recent version of process than what many versions of GHC are shipped with. @hvr, what are your thoughts on this?

@Mistuke
Copy link
Contributor Author

Mistuke commented Jun 9, 2019

Bumping the lower version bounds of process poses an interesting question with respect to hsc2hs's support window, since this patch requires a more recent version of process than what many versions of GHC are shipped with. @hvr, what are your thoughts on this?

Yeah, I thought it would be OK since the newer version of process should have been usable on older GHCs as well, but the CI proved me wrong here. I could instead do a soft bump. e.g. if an #ifdef on the process versions in the code and leave the bounds as is. This would fix it on the newer versions of GHC and at least won't regress the older versions.

@Mistuke
Copy link
Contributor Author

Mistuke commented Jun 9, 2019

Thanks for the review! I'll wait for what to do with the version bounds and will update the PR.

@RyanGlScott
Copy link
Member

Yeah, I thought it would be OK since the newer version of process should have been usable on older GHCs as well, but the CI proved me wrong here.

The CI can be a little misleading in this regard, as it's specifically configured to use the version of process that's bundled with GHC. We could tweak this setting so that cabal reinstalls a sufficiently new version of process, and that would fix the GHC 8.0.2 build failure at the very least. Even if we did this, however, process-1.5 requires base-4.4 (i.e., GHC 7.2) or later, which means that we'd have to drop GHC 7.0 support. This doesn't bother me that much, since GHC 7.0 is extremely old, and several other boot libraries have already moved to drop support for it.

What's more troublesome is the fact that the oldest version to actually be bundled with a sufficiently new version of process to support this patch is GHC 8.2, according to this chart. I'm not sure how likely this is to actually matter in practice, given that hsc2hs is an executable and is typically built separately from other components, so it seems unlikely that one would get locked into restrictive build plans as a result of this. But @hvr is much more knowledgeable in this area than I am, so I'll defer to his judgment.

@RyanGlScott
Copy link
Member

I talked with @hvr on IRC about this, and he would like to keep supporting build plans with old versions of process on non-Windows OSes if possible. He recommended adding the following to the .cabal file:

if os(windows)
  process    >= 1.5

And then guarding the use of use_process_jobs with #if defined(mingw32_HOST_OS) ... #endif. This way, we only have to limit build plans on Windows.

@Mistuke
Copy link
Contributor Author

Mistuke commented Jun 17, 2019

Thanks @RyanGlScott , I will update the PR tomorrow. Cheers.

@Mistuke
Copy link
Contributor Author

Mistuke commented Jun 18, 2019

Opened a request at directory haskell/directory#96

Copy link
Member

@RyanGlScott RyanGlScott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking better!

Copy link
Member

@RyanGlScott RyanGlScott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The build finally passes, yay!

Do you mind adding a brief summary of these changes to the changelog.md? Thanks!

Also, one more inline comment.

@RyanGlScott
Copy link
Member

Great! Just add a mention of these changes to changelog.md and I think we can consider this ready to land.

@Mistuke
Copy link
Contributor Author

Mistuke commented Jun 23, 2019

Right, sorry about that. completely forgot about the changelog... Too much to do :(

@RyanGlScott
Copy link
Member

No worries. Thanks for fixing this, @Mistuke!

@RyanGlScott RyanGlScott merged commit 0f68aff into haskell:master Jun 23, 2019
@Mistuke Mistuke deleted the gh-77-update-to-new-process branch June 23, 2019 14:16
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.

2 participants