Show constraint sources in dependency solver errors #10524

9999years · 2024-11-04T19:51:27Z

Before:

[__0] rejecting: memory-0.18.0 (constraint from user target requires ==0.17.0)

After:

[__0] rejecting: memory-0.18.0
  (constraint from cabal.project requires ==0.17.0)

First shot at #10519. Seems unavoidably large.

Builds off of #9578.

Future work: Propagate line numbers into these constraints!

Implementation strategy:

Propagate provenance (ProjectConfigPath/ConstraintSource) into project config types (Distribution.Client.ProjectConfig, D.C.ProjectConfig.Legacy)
Extract and attach that information in D.C.ProjectConfig.findProjectPackages
Attach that information to ProjectPackageLocation (returned from findProjectPackages). Note: Might be worth unifying that with PackageLocation, would make the new type PackageLocationProvenance.
Use that information to write better ConstraintSources into the solver; this is mostly implemented.

Overall it seems like all the information exists, but it takes a lot of work to propagate it into the right parts of the system.

We have a lot of `showType` functions that are effectively a `Pretty` instance but less composable. Let's make them proper `Pretty` instances. Split off of haskell#10524

cabal-testsuite/PackageTests/NewSdist/MultiTarget/all-test-sute.out

cabal-testsuite/PackageTests/ConstraintSource/SourceRepositoryPackage/cabal.out

ulysses4ever · 2024-11-09T18:56:40Z

@jasagredo just a heads-up: on the last Cabal meeting @9999years asked to not comment on their PRs while a PR is in the draft mode. I remember it because I’m guilty in it more than many!

9999years · 2024-11-18T20:37:38Z

Needs a release note.

Before: [__0] rejecting: memory-0.18.0 (constraint from user target requires ==0.17.0) After: [__0] rejecting: memory-0.18.0 (constraint from cabal.project requires ==0.17.0)

9999years · 2024-11-19T20:55:10Z

Added a release note and tests, addressed comments. Should be ready to go!

geekosaur

It's a pity 3.14.1.0 is breathing down our necks; it would be nice to see this go in, seems like it'd be a big UX improvement.

cabal-install/cabal-install.cabal

jasagredo · 2024-11-20T10:32:36Z

cabal-install-solver/src/Distribution/Solver/Types/WithConstraintSource.hs

+import Text.PrettyPrint
+
+-- | A package bundled with a `ConstraintSource`.
+data WithConstraintSource pkg =


I see this type being used as WithContraintSource UserConstraint so I think pkg is not actually a pkg? i.e. constraintPackage is misnamed?

Yeah, constraintPackage is often (but not always) a package. I wasn't sure of a better name for it. constraintInner, maybe? constraintConstraint is also somewhat misleadying, as it's a constraint source and not a UserConstraint itself...

I renamed them, how do these look?

constraintPackage → constraintInner

constraintConstraint → constraintSource

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

`constraintPackage` -> `constraintInner` `constraintConstraint` -> `constraintSource`

grayjay

I started reviewing this PR, but I had a question. It looks like a lot of the changes relate to adding ConstraintSources to targets in v2 commands. Why do the build targets need ConstraintSources? I had thought that targets for most commands only specified the subset of the project to build and didn't affect solving. The only exception I can think of is v2-install, which can solve for packages outside of the project (e.g., "cabal install text-2.0"), but then I would expect all targets to be able to use the same ConstraintSource (ConstraintSourceUserTarget), which I think is the current behavior on master.

grayjay · 2024-11-25T06:28:23Z

cabal-install-solver/src/Distribution/Solver/Types/ConstraintSource.hs

+  -- | An implicit constraint added by Cabal.
+  | ConstraintSourceImplicit


Should this constructor be named ConstraintSourceImplicitTarget, to match the Pretty instance? It would also help to add more detail to this comment.

grayjay · 2024-11-25T06:32:54Z

cabal-install-solver/src/Distribution/Solver/Types/ConstraintSource.hs

+
+  -- | An implicit constraint added by Cabal.
+  | ConstraintSourceImplicit
+  deriving (Show, Eq, Ord, Generic, Typeable)


Is the Typeable instance necessary? The same applies to NamedPackage and WithConstraintSource.

grayjay · 2024-11-25T21:13:05Z

cabal-install-solver/src/Distribution/Solver/Types/WithConstraintSource.hs

+import Text.PrettyPrint
+
+-- | A package bundled with a `ConstraintSource`.
+data WithConstraintSource pkg =


WithConstraintSource looks like a generalization of LabeledPackageConstraint. Is it meant to replace LabeledPackageConstraint? That's probably too much to do in one PR, though.

grayjay · 2024-11-25T21:17:36Z

cabal-install-solver/src/Distribution/Solver/Types/SourcePackage.hs

@@ -25,7 +27,7 @@ data SourcePackage loc = SourcePackage
  , srcpkgSource        :: loc
  , srcpkgDescrOverride :: PackageDescriptionOverride
  }
-  deriving (Eq, Show, Generic, Typeable)
+  deriving (Eq, Show, Functor, Generic, Typeable)


Why is this Functor instance needed?

grayjay · 2024-11-25T21:43:57Z

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

+      =<< readTargetSelectors
+        (localPackages baseCtx)
+        (Just BenchKind)
+        (map (\target -> WithConstraintSource{constraintInner = target, constraintSource = ConstraintSourceCommandlineFlag}) targetStrings)


Why does this line use ConstraintSourceCommandlineFlag as the ConstraintSource? If I understand correctly, targetStrings is a list of targets provided to the bench command, so ConstraintSourceUserTarget seems like a better match. This comment also applies to many of the other commands.

grayjay · 2024-11-26T04:48:23Z

cabal-install/tests/UnitTests/Distribution/Solver/Modular/DSL.hs

+                ( WithConstraintSource
+                    { constraintInner =
+                        NamedPackage (C.mkPackageName p) []
+                    , constraintSource = ConstraintSourceUnknown


Should this be ConstraintSourceUserTarget?

grayjay · 2024-11-26T04:59:53Z

cabal-testsuite/PackageTests/ConstraintSource/SourceRepositoryPackage/cabal.project

+-- This is `my-lib` 0.9.
+source-repository-package
+  type: git
+  location: https://github.com/9999years/cabal-testsuite-my-lib.git
+  tag: 9a0af0aa81325c71e744def11db06265840ffb5f


Could this test use an existing package in the Haskell organization as in the postCheckoutCommand test?

cabal/cabal-testsuite/PackageTests/postCheckoutCommand/cabal.positive.project

Lines 3 to 8 in 02da008

source-repository-package

type: git

-- A Sample repo to test post-checkout-command

location: https://github.com/haskell/bytestring

post-checkout-command: true

tag: 0.10.9.0

grayjay · 2024-11-26T07:38:08Z

cabal-testsuite/PackageTests/ConstraintSource/SourceRepositoryPackage/cabal.out

+Could not resolve dependencies:
+[__0] next goal: my-lib (user goal)
+[__0] rejecting: my-lib; 2.0, 1.0
+      (constraint from cabal.project requires ==0.9)


Similarly to the message in use-local-version-of-package.out, I think that this message would be clearer if it explained that the constraint came from the version of a package listed in cabal.project. It could also help to mention that it is a source repository package.

grayjay · 2024-11-26T20:52:05Z

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

-  , configExConstraints :: [(UserConstraint, ConstraintSource)]
-  , configPreferences :: [PackageVersionConstraint]
+  , configExConstraints :: [WithConstraintSource UserConstraint]
+  , configPreferences :: [WithConstraintSource PackageVersionConstraint]


Here is the field where ConstraintSources are applied to preferences, which I mentioned in the meeting. The dependency solver currently doesn't describe preferences in error messages, so I think that it would take a lot more work to make this useful. I think that it should either be removed or split into a separate PR.

grayjay · 2024-11-26T23:20:30Z

cabal-testsuite/PackageTests/NewSdist/MultiTarget/all-test-suite.test.hs

Did you mean to also change the content of this test?

9999years force-pushed the dependency-provenance branch 2 times, most recently from 4811a3c to 0c8471d Compare November 4, 2024 22:29

9999years mentioned this pull request Nov 4, 2024

Convert display functions into Pretty instances #10526

Open

2 tasks

9999years force-pushed the dependency-provenance branch 9 times, most recently from 9ebbba8 to 522bec9 Compare November 9, 2024 00:25

jasagredo reviewed Nov 9, 2024

View reviewed changes

cabal-testsuite/PackageTests/NewSdist/MultiTarget/all-test-sute.out Outdated Show resolved Hide resolved

cabal-testsuite/PackageTests/ConstraintSource/SourceRepositoryPackage/cabal.out Show resolved Hide resolved

9999years force-pushed the dependency-provenance branch from 522bec9 to e52d016 Compare November 18, 2024 19:59

9999years changed the title ~~[WIP] Show constraint sources in dependency solver errors~~ Show constraint sources in dependency solver errors Nov 18, 2024

9999years marked this pull request as ready for review November 18, 2024 19:59

9999years force-pushed the dependency-provenance branch 2 times, most recently from c10bd7d to 9854c9d Compare November 18, 2024 20:37

9999years force-pushed the dependency-provenance branch from 9854c9d to 9492b41 Compare November 18, 2024 23:40

9999years added 2 commits November 18, 2024 16:40

Add ConstraintSource to local packages

bc8716a

Before: [__0] rejecting: memory-0.18.0 (constraint from user target requires ==0.17.0) After: [__0] rejecting: memory-0.18.0 (constraint from cabal.project requires ==0.17.0)

Add tests

b27b413

9999years force-pushed the dependency-provenance branch from 9492b41 to b27b413 Compare November 19, 2024 00:45

9999years requested review from jasagredo, philderbeast and ulysses4ever November 19, 2024 20:33

geekosaur approved these changes Nov 19, 2024

View reviewed changes

cabal-install/cabal-install.cabal Show resolved Hide resolved

ulysses4ever requested a review from grayjay November 20, 2024 02:42

jasagredo reviewed Nov 20, 2024

View reviewed changes

9999years added 2 commits November 20, 2024 09:59

Use local binding to shorten buildAction line

44fcf38

Rename WithConstraintSource fields

db60ab4

`constraintPackage` -> `constraintInner` `constraintConstraint` -> `constraintSource`

9999years mentioned this pull request Nov 25, 2024

PR template: Make it better! #10594

Open

grayjay reviewed Nov 26, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Show constraint sources in dependency solver errors #10524

Show constraint sources in dependency solver errors #10524

9999years commented Nov 4, 2024 •

edited

Loading

ulysses4ever commented Nov 9, 2024

9999years commented Nov 18, 2024

9999years commented Nov 19, 2024

geekosaur left a comment

jasagredo Nov 20, 2024

9999years Nov 20, 2024

9999years Nov 20, 2024

grayjay left a comment

grayjay Nov 25, 2024

grayjay Nov 25, 2024

grayjay Nov 25, 2024

grayjay Nov 25, 2024

grayjay Nov 25, 2024

grayjay Nov 26, 2024

grayjay Nov 26, 2024

grayjay Nov 26, 2024

grayjay Nov 26, 2024

grayjay Nov 26, 2024

		-- \| An implicit constraint added by Cabal.
		\| ConstraintSourceImplicit

	source-repository-package
	type: git
	-- A Sample repo to test post-checkout-command
	location: https://github.com/haskell/bytestring
	post-checkout-command: true
	tag: 0.10.9.0

Show constraint sources in dependency solver errors #10524

Are you sure you want to change the base?

Show constraint sources in dependency solver errors #10524

Conversation

9999years commented Nov 4, 2024 • edited Loading

ulysses4ever commented Nov 9, 2024

9999years commented Nov 18, 2024

9999years commented Nov 19, 2024

geekosaur left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grayjay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

9999years commented Nov 4, 2024 •

edited

Loading