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 #5639 Backport efficient ghc-pkg unregister #6222

Merged
merged 17 commits into from
Sep 13, 2023
Merged

Fix #5639 Backport efficient ghc-pkg unregister #6222

merged 17 commits into from
Sep 13, 2023

Conversation

mpilgrem
Copy link
Member

@mpilgrem mpilgrem commented Aug 28, 2023

This pull request builds on top of pull request #6226.

  • Any changes that could be relevant to users have been recorded in ChangeLog.md.
  • The documentation has been updated, if necessary

Please also shortly describe how you tested your change. Bonus points for added tests! Testing locally. Although this appears to work for GHC 9.4.6, it currently fails with early versions of GHC, such as GHC 8.0.1.

On Windows 11 and Windows Terminal:

GHC Snapshot Cabal Install GHC Test
8.0.1 lts-7.24 Fails S-6374 N/A
8.2.2 lts-11.22 Fails S-6374 N/A
8.4.4 lts-12.26 Fails S-6374 N/A
8.6.5 lts-14.27 Fails S-6374 N/A
8.8.4 lts-16.31 Fails S-6374 N/A
8.10.7 lts-18.28 Fails S-6374 N/A
9.0.2 lts-19.33 3.4.1.0 Fails Warning
9.2.8 lts-20.26 3.6.3.0 Fails Warning
9.4.6 lts-21.9 3.8.1.0 Works
9.6.2 nightly-2023-08-29 3.10.1.0 Works

The test is a simple two-package project, where packageB depends on packageA, and the version of packageA is changed, prompting both to be unregistered.

The S-6374 errors before GHC 9.0.2 are likely a manifestation of this GHC bug: https://gitlab.haskell.org/ghc/ghc/-/issues/19452. The workaround was to delete the setup-exec-cache and setup-exe-src in the Stack root.

'Warning' refers to output like:

❯ stack build
packageA-0.1.0.0: unregistering
packageB-0.1.0.0: unregistering

Warning: D:\Users\mike\Code\Haskell\unregTest\.stack-work\install\9bd81736\pkgdb\package.cache: withBinaryFile: inappropriate type (not enough bytes)
...
Error: [S-7282]
       Stack failed to execute the build plan.
       ...
       Error: [S-6739]
       The impossible happened! singleBuild: multiple results when describing installed package ...

The inappropriate type (not enough bytes) message can be traced, I think, to GHC.Unit.Database.decodeFromFile, used by readPackageDbForGhcPkg.

I also think the underlying problem is that Distribution.Types.InstalledPackageInfo.InstalledPackageInfo from the Cabal (or Cabal-syntax) package has changed from one version of Cabal to another. Extracts:

GhcPkg.readPackageDbForGhcPkg cache mode 
  >>= uncurry mkPackageDB
...
mkPackageDB :: [InstalledPackageInfo]
            -> GhcPkg.DbOpenMode mode GhcPkg.PackageDbLock
            -> IO (PackageDB mode)

@mpilgrem mpilgrem marked this pull request as draft August 30, 2023 22:16
@mpilgrem mpilgrem marked this pull request as ready for review September 1, 2023 21:06
@mpilgrem mpilgrem force-pushed the fix5639 branch 5 times, most recently from 01cea93 to 73825c9 Compare September 2, 2023 23:48
@mpilgrem
Copy link
Member Author

mpilgrem commented Sep 3, 2023

I think the current version of this pull request works as intended.

On Windows 11 and Windows Terminal:

GHC Snapshot Cabal Install GHC Test
8.0.1 lts-7.24 1.24.0.0  Works
8.2.2 lts-11.22 2.0.1.0  Works
8.4.4 lts-12.26 2.2.0.1  Works
8.6.5 lts-14.27 2.4.1.0  Works
8.8.4 lts-16.31 3.0.1.0  Works
8.10.7 lts-18.28 3.2.1.0  Works
9.0.2 lts-19.33 3.4.1.0 Works
9.2.8 lts-20.26 3.6.3.0 Works
9.4.6 lts-21.9 3.8.1.0 Works
9.6.2 nightly-2023-08-29 3.10.1.0 Works

On Ubuntu (on WSL2) and Windows Terminal:

GHC Snapshot Cabal Install GHC Test
8.0.1 lts-7.24 1.24.0.0  Works
9.2.8 lts-20.26 3.6.3.0 Works
9.4.6 lts-21.9 3.8.1.0 Works
9.6.2 nightly-2023-08-29 3.10.1.0 Works

@mpilgrem
Copy link
Member Author

mpilgrem commented Sep 3, 2023

@juhp, this version of Stack should use an efficient bulk unregister of packages. I am hoping you are in a position to 'stress test' it with Stackage in a way that I am not. In particular, does it result in the time saving in #5639 that you were anticipating?

@mpilgrem
Copy link
Member Author

mpilgrem commented Sep 3, 2023

Rebased on revised #6222.

@mpilgrem mpilgrem force-pushed the fix5639 branch 2 times, most recently from 9359b5c to a7c1106 Compare September 3, 2023 19:41
@juhp
Copy link
Contributor

juhp commented Sep 4, 2023

Thanks @mpilgrem 🙏 - sounds great and really exciting
Yes I think we can test this (maybe even in production on Nightly).
Is it possible to get a test build - dunno if the CI generates such an artifact?
If so we could just directly pull it down for nightly builds, or we can probably hack around it.
(I won't personally be on Stackage duty until a few weeks later, but I will share this with the Stackage team now anyway.)

@mpilgrem
Copy link
Member Author

mpilgrem commented Sep 4, 2023

@juhp, on GitHub's web interface, if you click on the 'Checks' tab in the menu bar near the top of this PR page, and then the '> Integration tests' check in the side bar on the left of the resulting page, then at the bottom of the resulting screen you should have populated 'Artifacts Produced during runtime' - those are archive files with the bindists that Stack would publish if they were final versions. So, under 'Linux' (for example), is Linux.zip, containing stack-2.12.0-linux-x86_64.tar.gz, which contains (when unpacked) a stack executable in directory stack-2.12.0-linux-x86_64.

@mpilgrem mpilgrem force-pushed the fix5639 branch 2 times, most recently from f0e327e to 339b082 Compare September 9, 2023 15:22
@juhp
Copy link
Contributor

juhp commented Sep 12, 2023

I am currently testing this for Stackage Nightly.

In this first run, unregistering seemed to go fairly quickly - though I didn't watch it happen.
Also curator doesn't time the different steps/phases...
but I think indeed it appears to be working well enough so far.

@juhp
Copy link
Contributor

juhp commented Sep 13, 2023

So I think it fine to merge from my pov at least, if you don't have any reservations of course

@juhp
Copy link
Contributor

juhp commented Sep 13, 2023

The difference in speed is like day and night - if one closes one's eyes too long, one can easily miss it now.

@mpilgrem mpilgrem merged commit 4eadbac into master Sep 13, 2023
@mpilgrem mpilgrem deleted the fix5639 branch September 13, 2023 16:40
@juhp
Copy link
Contributor

juhp commented Sep 14, 2023

Now, I am curious though, do you plan to release 2.13 any time soon?

@mpilgrem
Copy link
Member Author

I was going to start the process this weekend.

@juhp
Copy link
Contributor

juhp commented Sep 14, 2023

Wow thanks, that's great! - dunno if it should be 3.1 even? (one reason for that could be the change from Cabal to ghc versions under .stack-work/dist/x86_64-linux-tinfo6) Anyway probably not the right place to discuss that :-)

@mpilgrem
Copy link
Member Author

I was saving Stack 3.1 for when Stack supported public sublibraries ...

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