Combine non-installable & non-upgradable packages #8712

wismill · 2023-01-30T08:55:23Z

Combine non-installable and non-upgradable packages list.

Since the two lists are synced, we try to keep the list in Distribution.Client.Dependency (preferred location) and pass it as a parameter to functions in Distribution.Solver.Modular.Solver.

Fixes #8581

Please include the following checklist in your PR:

Patches conform to the coding conventions.
Any changes that could be relevant to users have been recorded in the changelog.
The documentation has been updated, if necessary.

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

cabal-install/src/Distribution/Client/Dependency.hs

grayjay

Thanks for simplifying the non-upgradeable package constraints! This PR is a big improvement and can be merged as is, though I think it could go further to resolve #8581 by also removing the requireInstalled function. The current PR is a straightforward refactoring and doesn't need any additional testing, but removing requireInstalled has the potential to change behavior, so it might help to add more unit tests if you choose to remove that function.

Co-authored-by: Artem Pelenitsyn <[email protected]>

wismill · 2023-02-01T10:14:56Z

@grayjay I gave a try removing requireInstalled. Let us see if it passes the test suite as it is. I should remove the field allowBootLibInstalls in SolverConfig as well.

There are already some tests in cabal-install/tests/UnitTests/Distribution/Solver/Modular/Solver.hs. I will need help with use case to design new tests.

ulysses4ever · 2023-02-01T13:09:04Z

It may have been wiser to do the second part (requireInstalled business) as a separate PR. That way we could merge this faster.

This reverts commit adfce24.

wismill · 2023-02-01T13:34:01Z

Yes, I can try a separate PR. I reverted the commit.

As I suspected, it is not as simple as to remove requireInstalled (see failed tests).

@grayjay I think we need to add constraints in a similar way than dontUpgradeNonUpgradeablePackages, don’t we?

ulysses4ever · 2023-02-01T16:44:12Z

cabal-install/src/Distribution/Client/Dependency.hs

@@ -710,7 +717,8 @@ resolveDependencies platform comp pkgConfigDB solver params =
  $ fmap (validateSolverResult platform comp indGoals)
  $ runSolver solver (SolverConfig reordGoals cntConflicts fineGrained minimize
                      indGoals noReinstalls
-                      shadowing strFlags allowBootLibs onlyConstrained_ maxBkjumps enableBj
+                      shadowing strFlags allowBootLibs nonUpgradeablePackages


Would it be possible to add some comment as to why the nonInstallablePackages field is initialized with nonUpgradeablePackages (there’s a clear mismatch here)? Perhaps, reference a place where the difference between noninstallable and nonupgradeable is discussed? @grayjay, do you happen to know such a place? Or if there’s none, perhaps you could help with writing the comment I’m asking for. This stuff is very confusing and I hope we can write up something that could clear things up for the future reference.

@ulysses4ever I think that non-installable and non-upgradable may have the exact same meaning, which is the reason I opened #8581. If this PR is merged as is, then I think it can just have a comment pointing to #8581. If it is possible to remove requireInstalled, then this part of the change can be reverted.

I added a comment

If we agree that "non-installable" and "non-upgradable" are the same, can we please rename one of these into the other?

@wismill thanks for the comment. Can you also update the comment on nonUpgradeablePackages to say that they are the same?

@ulysses4ever I'm not completely sure that non-installable and non-upgradable are the same. I wasn't able to determine the difference by looking at the history of the code, so I think that the best way to answer the question is to try to remove one of them and see whether the tests pass. I think it would be better to just link to issue #8581 until we can try removing the rest of the redundant code.

I think the current code fullfils @grayjay comment, so I will wait you reach an agreement 😉.
Once this PR is merged, I can try the proposed follow-up.

grayjay · 2023-02-04T02:17:25Z

@wismill Thanks for trying to remove requireInstalled. I think it would be better to merge this PR with only the refactoring change first, since there was a test failure.

It looks like the test failure could have been caused by this line:

cabal/cabal-install/src/Distribution/Client/Dependency.hs

Line 397 in a5ddb14

| Set.notMember (mkPackageName "base") (depResolverTargets params)

I'm not sure what the purpose of that line was, but I'm guessing that it was made obsolete by the flag --allow-boot-library-installs and can be removed.

There are already some tests in cabal-install/tests/UnitTests/Distribution/Solver/Modular/Solver.hs. I will need help with use case to design new tests.

Testing for a larger part of the error message in "Refuse to install base without --allow-boot-library-installs" is probably enough.

@grayjay I think we need to add constraints in a similar way than dontUpgradeNonUpgradeablePackages, don’t we?

I don't think we need to add any more constraints than the ones in dontUpgradeNonUpgradeablePackages. They already prevent that list of packages from being installed.

ulysses4ever · 2023-02-11T16:47:11Z

@wismill after grepping the source, I have another proposal that may help to lower my confusion. Could you change the name of the binding you introduced as nonUpgradeable to nonInstallable? My reasoning is that I firmly believe that if we try to stick to one name (non-installable v non-upgradeable) it would be easier to understand the source. And currently, the source seems to use ~~non-upgradeable~~ non-installable more often.

I'd also suggest to change cabal-install-solver/src/Distribution/Solver/Types/ConstraintSource.hs:64 from non-upgradeable to non-installable.

grayjay · 2023-02-12T08:04:14Z

@ulysses4ever I don't think that we should rename the variables now, since we don't know whether non-installable and non-upgradable are the same. This PR only resolves #8581 partially, so the comments can link to that issue. The next step is to determine whether non-installable and non-upgradable packages are treated the same, and, if they are, remove requireInstalled and related code. A clearer name for the common list of packages in this PR could be nonInstallableAndNonUpgradeablePackages.

wismill · 2023-02-12T08:21:43Z

@grayjay I think what you propose is a good path for a transition. With good comments in the code it could be reverse if it turns out that the two lists may be different, now or in the future.

Who should we ask to get a definitive answer? I have the feeling GHC devs could enlighten us.

Should this PR be merged soon, I might get another one ready in time for Cabal 3.10 to fix #7993.

What is the release schedule?

wismill · 2023-02-12T08:58:18Z

Some findings:

How stack manages wired-in packages. wired-in seems another important keyword!
Doc about wired-in libraries in GHC:
- compiler/GHC/Unit.hs
- compiler/GHC/Unit/Types.hs
Libraries with ghc-options: -this-unit-id in GHC source:
- base
- ghc
- ghc-bignum
- ghc-prim
- rts
- template-haskell

grayjay · 2023-02-23T08:56:27Z

Who should we ask to get a definitive answer? I have the feeling GHC devs could enlighten us.

GHC devs could help with creating a complete list of packages that shouldn't be reinstalled and using the correct terminology (non-upgradable vs wired-in vs boot). However, I think that having separate non-installable and non-upgradable lists is only an implementation detail of the cabal dependency solver and doesn't relate to GHC.

gbaz · 2023-02-23T18:29:34Z

Approving as a good incremental step.

Mikolaj · 2023-02-23T19:20:38Z

@wismill: if you add the "squash+merge_me" label, it should automatically rebase as needed.

Combine non-installable & non-upgradable packages

45aba44

wismill force-pushed the wip/fix8581 branch from d415623 to 45aba44 Compare January 30, 2023 09:03

wismill requested a review from grayjay January 30, 2023 09:32

wismill marked this pull request as ready for review January 30, 2023 13:16

ulysses4ever reviewed Jan 30, 2023

View reviewed changes

cabal-install/src/Distribution/Client/Dependency.hs Outdated Show resolved Hide resolved

grayjay added the cabal-install: solver label Jan 31, 2023

grayjay approved these changes Jan 31, 2023

View reviewed changes

wismill and others added 2 commits February 1, 2023 10:15

Typo

feb52ad

Co-authored-by: Artem Pelenitsyn <[email protected]>

Remove requireInstalled

adfce24

Revert "Remove requireInstalled"

38785a6

This reverts commit adfce24.

ulysses4ever reviewed Feb 1, 2023

View reviewed changes

Comment only [skip ci]

3f9c53c

grayjay approved these changes Feb 4, 2023

View reviewed changes

ulysses4ever added the attention: needs-review label Feb 7, 2023

gbaz approved these changes Feb 23, 2023

View reviewed changes

andreasabel approved these changes Feb 23, 2023

View reviewed changes

andreasabel removed the attention: needs-review label Feb 23, 2023

Merge branch 'master' into wip/fix8581

2a032fa

wismill added the squash+merge me Tell Mergify Bot to squash-merge label Feb 26, 2023

mergify bot added the merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days label Feb 28, 2023

Merge branch 'master' into wip/fix8581

88e768b

mergify bot merged commit b016e59 into haskell:master Feb 28, 2023

grayjay mentioned this pull request Jun 26, 2023

ghc-boot is reinstallable #9064

Closed

Combine non-installable & non-upgradable packages #8712

Combine non-installable & non-upgradable packages #8712

Uh oh!

Conversation

wismill commented Jan 30, 2023 • edited by ulysses4ever Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

grayjay left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wismill commented Feb 1, 2023

Uh oh!

ulysses4ever commented Feb 1, 2023

Uh oh!

wismill commented Feb 1, 2023

Uh oh!

ulysses4ever Feb 1, 2023

Choose a reason for hiding this comment

Uh oh!

grayjay Feb 4, 2023

Choose a reason for hiding this comment

Uh oh!

wismill Feb 4, 2023

Choose a reason for hiding this comment

Uh oh!

ulysses4ever Feb 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

grayjay Feb 5, 2023

Choose a reason for hiding this comment

Uh oh!

wismill Feb 5, 2023

Choose a reason for hiding this comment

Uh oh!

grayjay commented Feb 4, 2023

Uh oh!

ulysses4ever commented Feb 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

grayjay commented Feb 12, 2023

Uh oh!

wismill commented Feb 12, 2023

Uh oh!

wismill commented Feb 12, 2023

Uh oh!

grayjay commented Feb 23, 2023

Uh oh!

gbaz commented Feb 23, 2023

Uh oh!

Mikolaj commented Feb 23, 2023 • edited by andreasabel Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

wismill commented Jan 30, 2023 •

edited by ulysses4ever

Loading

grayjay left a comment •

edited

Loading

ulysses4ever Feb 4, 2023 •

edited

Loading

ulysses4ever commented Feb 11, 2023 •

edited

Loading

Mikolaj commented Feb 23, 2023 •

edited by andreasabel

Loading