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

Fix cabal on Windows 7 #7844

Merged
merged 4 commits into from
Dec 3, 2021
Merged

Fix cabal on Windows 7 #7844

merged 4 commits into from
Dec 3, 2021

Conversation

sergv
Copy link
Collaborator

@sergv sergv commented Nov 30, 2021


Please include the following checklist in your PR:

Please also shortly describe how you tested your change. Bonus points for added tests!


I have encountered severe issue on Windows 7. cabal executable built with process 1.6.9 or later is unable to build any packages. The issue manifests itself as cabal being unable to determine version of the ghc but the actual issue is that it's process spawning mechanic is faulty.

The investigation revealed following: starting with process 1.6.9 the Cabal library enables so-called jobs for newly created processes. This is done to better detect when a spawned process terminates. Simplistically, a job is a set of processes tracked by the OS. When a relevant relevant is supplied, the processes spawned by the process package are assigned a fresh job and any children they spawn will inherit that job. Later on the job can be checked for whether all the processes in it terminated.

The issue on Windows 7 (and earlier versions as well, but that's less relevant) is that a process can have at most one job assigned to it1 which breaks when a job is inherited automatically. I.e. spawned processes are unable to use job management for their own children.

Specifically, I invoke cabal build which spawns cabal act-as-setup process and assigns a job to it. The cabal act-as-setup tries to spawn more processes, like ghc --numeric-version but cannot assign fresh jobs to them since they already inherit one. The process cannot be started and build does not proceed.

Thus the proposed fix is to disable job management on Windows 7 or earlier. On later Windows versions it's fine since it allows processes to have multiple jobs provided they're included into one another.

Footnotes

  1. Relevant exceprt from https://docs.microsoft.com/en-us/windows/win32/api/jobapi2/nf-jobapi2-assignprocesstojobobject:

    Windows 7, Windows Server 2008 R2, Windows XP with SP3, Windows Server 2008, Windows Vista and Windows Server 2003: The process must not already be assigned to a job; if it is, the function fails with ERROR_ACCESS_DENIED. This behavior changed starting in Windows 8 and Windows Server 2012.

@Mikolaj
Copy link
Member

Mikolaj commented Nov 30, 2021

@Mistuke: would you have a super-quick look?

@jneira
Copy link
Member

jneira commented Dec 1, 2021

As a occasional windows 7 user i have to say this is really great, many thanks
Not sure if you are aware but he issue was reported and analyzed in #7309, so this would close that issue.
A ghc ticket was opened but it is stalled (almost a won't fix).

Considerations:

  • there is a direct workaround, described in that issue, while cabal is buildable with ghc < 8.8.4 (the first ghc with process-1.6.9): cabal install cabal-install --constraints="process < 1.6.9.0" -w ghc-8.6.5
  • it is unfortunate to have to include cpp depending on the windows version in the cabal codebase, for a general issue which affects all executables built with process >=1.6.9
  • windows 7 reached eol quite time ago and it is not officially supported by ghc anymore
    • but while cabal supports be built with ghcs supporting windows 7 i think makes sense include the hack
  • but well, compat modules are designed to contain those kind of hacks, so it is assumible imo. However we should think in some expiration date for the hack to eventually remove it, and add a comment about
    • that expiration date could be precisely the day cabal will not support ghcs supporting windows 7

@jneira jneira linked an issue Dec 1, 2021 that may be closed by this pull request
@Mistuke
Copy link
Collaborator

Mistuke commented Dec 1, 2021

This looks fine in that windows 7 will run, but it should be made clear that it still suffers from the race conditions which the process jobs were introduced to deal with.

That is, if cabal calls a posix application that uses exec the behavior is unpredictable or if calling an older ghc which also doesn't have process jobs enabled.

But we can't do much for Windows 7 anyway as this is an OS bug that Microsoft isn't fixing. So this is probably as good as it gets.

@Mikolaj
Copy link
Member

Mikolaj commented Dec 1, 2021

@Mistuke, @jneira: thank you!

@Bodigrim
Copy link
Collaborator

Bodigrim commented Dec 1, 2021

windows 7 reached eol quite time ago

Unfortunately this is not quite so: Windows 7 will receive Extended Security Updates support until October 2024. It still holds ~15% of Windows market, especially prominent in corporate sector. As a poor soul stuck with Windows 7 myself, I cannot underline enough how greatly I would benefit from this patch, because otherwise the last working official release is 3.2.

I tested the patch on Win7 machine and can confirm that it works fine.

@Mikolaj could we please have it merged and released as 3.6.3.0?

@Mikolaj
Copy link
Member

Mikolaj commented Dec 1, 2021

All right! Let's merge it then. If there's 3.6.3.0, I'm pretty sure it's going to be included, but I'm not in charge of releases. @emilypi is.

@Mistuke
Copy link
Collaborator

Mistuke commented Dec 1, 2021

windows 7 reached eol quite time ago

Unfortunately this is not quite so: Windows 7 will receive Extended Security Updates support until October 2024.

ESU end date for Windows 7 is January 2023 [1] only windows embedded and Windows server go past that.

ESU is also expensive as sin and not available to the general public and is intended for people running legacy software whom are unable to upgrade this software and are willing to pay for continued support.

I have nothing against this going in but "working for me" is.. Anecdotal and Windows 7 most definitely is way out of support.

[1] https://docs.microsoft.com/en-us/lifecycle/faq/extended-security-updates#esu-availability-and-end-dates

Copy link
Member

@Mikolaj Mikolaj left a comment

Choose a reason for hiding this comment

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

It looks securely guarded with CPP and conditionals, so LGTM when used outside Windows 7.

@Mikolaj Mikolaj added the merge me Tell Mergify Bot to merge label Dec 1, 2021
@Mikolaj
Copy link
Member

Mikolaj commented Dec 1, 2021

* it is unfortunate to have to include cpp depending on the windows version in the cabal codebase, for a general issue which affects all executables built with process >=1.6.9

I think CPP does not depend on windows version, only the actual code run for any Windows does. But it's rather simple and readable (and I hope correct!).

  * that expiration date could be precisely the day cabal will not support ghcs supporting windows 7

Agreed. Or perhaps "not support ghcs that de facto work with Windows 7" regardless of official support? I don't know if there's a difference.

@jneira
Copy link
Member

jneira commented Dec 2, 2021

well I want to note again that the workaround of building from source always has been available and will do while cabal supports a ghc allowing builds with process <= 1.6.9. And I guess it will be several years.

I would ask the author to add a comment about when the hack could be removed, mentioning explicitly the ghc version if possible, to be discoverable in deprecation code changes

@Mikolaj Mikolaj added attention: needs-backport 3.6 and removed merge me Tell Mergify Bot to merge labels Dec 2, 2021
@Bodigrim
Copy link
Collaborator

Bodigrim commented Dec 2, 2021

I would ask the author to add a comment about when the hack could be removed, mentioning explicitly the ghc version if possible, to be discoverable in deprecation code changes

@jneira could you possibly please elaborate? https://gitlab.haskell.org/ghc/ghc/-/wikis/platforms/windows does not reveal anything, while GHC 9.0.1 release notes say that Windows 7 is supported and 9.2.1 notes does not claim an end of support (and indeed works fine). Which GHC versions do you have in mind?

@jneira
Copy link
Member

jneira commented Dec 3, 2021

thanks for checking actual support of ghc for win 7: if ghc still supports win 7 (or rather it did not deprecate it explicitly) there is no version to mention.

so I would add something like:

this exception, needed to support windows 7, should not be removed while cabal supports a ghc supporting windows 7 itself

It will mark when the hack can (and can not!) be removed


However I think win 7 support is suspended or something alike, cause this hack could be done upstream in process itself in other case, for all executables and not only cabal (see the ghc ticket linked in the issue about: https://gitlab.haskell.org/ghc/ghc/-/issues/19473)

@sergv
Copy link
Collaborator Author

sergv commented Dec 3, 2021

@jneira

hack could be done upstream in process itself in other case, for all executables and not only cabal

I think the process library shouldn't do this hack since it's perfectly good even on Windows 7 to use jobs for programs that don't use jobs for starting their own child processes.

sergv added 4 commits December 3, 2021 20:14
If they’re enabled on Windows 7 the cabal spawned with "actas-setup"
flag is not able to spawn any processes since it already inherits a
job from the toplevel cabal.
@jneira
Copy link
Member

jneira commented Dec 3, 2021

I think the process library shouldn't do this hack since it's perfectly good even on Windows 7 to use jobs for programs that don't use jobs for starting their own child processes.

And the hack makes any harm to those programs not starting child processes? cause if it does not, the hack only would fix the broken programs

EDIT: well i suppose the drawback would be the described above #7844 (comment) so fair enough. I suppose we can live with the consequences of those race conditions in cabal.

@Mikolaj Mikolaj added the merge me Tell Mergify Bot to merge label Dec 3, 2021
Copy link
Member

@jneira jneira left a comment

Choose a reason for hiding this comment

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

Many thanks for the fix. As i said i use windows 7 and will not have to build cabal from source anymore.

@mergify mergify bot merged commit 7313c3b into haskell:master Dec 3, 2021
@Mikolaj
Copy link
Member

Mikolaj commented Dec 3, 2021

@Mergifyio backport 3.6

@mergify
Copy link
Contributor

mergify bot commented Dec 3, 2021

backport 3.6

✅ Backports have been created

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cabal 3.4.0.0 is broken on Windows 7 (use_process_jobs vs. hsc2hs.exe)
5 participants