haskell
Show import tree provenance
Fixes #9562. This adds the path in the project import tree with the constraint conflict just below the solver's rejecting message (sample taken from the change log entry).
$ cabal build all --dry-run
...
[__1] next goal: hashable
[__1] rejecting: hashable-1.4.3.0
(<ROOT>/1-web-import-constraints.project requires ==1.4.2.0)
[__1] rejecting: hashable-1.4.2.0
(https://www.stackage.org/nightly-2023-12-07/cabal.config requires ==1.4.3.0)
imported by: <ROOT>/1-web-import-constraints.project
[!IMPORTANT] The way imports worked changed between
cabal-install-3.8andcabal-install-3.10. With the earlier version imports were anchored to the project root (relative to a fixed location) but this was changed to make them relative to their importer. I had to pay close attention to the check for seen imports, something I added tests for. These tests merged already with #9665 and #9680. I've also added verbose logging at theinfolevel so we can see the import we're processing, the seen imports and the import we're fetching.Because relative imports may contain
..and the only way to resolve this is to use canonicalizePath I had to take an extra dependency ondirectoryincabal-install-solver. I also needednetwork-urito check for URIs and avoid messing with them. I assume that once we import a URI config then imports stop there. Put another way, something imported viahttps://...doesn't itself import another config. Those extra dependencies we should be able to remove when the suggested change of @andreabedini lands, #9642.
- [x] Patches conform to the coding conventions.
- [x] Any changes that could be relevant to users have been recorded in the changelog.
- [ ] ~The documentation has been updated, if necessary.~ (Can't find "rejecting" in the docs to change).
- [x] Manual QA notes have been included.
- [x] Tests have been added. (I took stripped down tests that I hope to reuse and expand for #9510).
If testing manually, I suggest trying cabal build --dry-run with a simple cabal.project, a project with imports and a bare package with no project. To see the change, a solver rejection will need to be triggered by introducing a version constraint.
[!NOTE] I'd tried to use a tree-like layout but settled on a list instead after review.
I used
+--ASCII chars to mark tree nodes (and not the same ones as tree does). Can we use the nicer unicode chars?$ tree -P 'cabal.project*' --prune -L 1 --charset ascii . |-- cabal.project |-- cabal.project.buildinfo |-- cabal.project.coverage |-- cabal.project.doctest |-- cabal.project.latest-ghc |-- cabal.project.libonly |-- cabal.project.meta |-- cabal.project.release |-- cabal.project.validate |-- cabal.project.validate.libonly `-- cabal.project.weeder $ tree -P 'cabal.project*' --prune -L 1 --charset utf8 . ├── cabal.project ├── cabal.project.buildinfo ├── cabal.project.coverage ├── cabal.project.doctest ├── cabal.project.latest-ghc ├── cabal.project.libonly ├── cabal.project.meta ├── cabal.project.release ├── cabal.project.validate ├── cabal.project.validate.libonly └── cabal.project.weeder
@andreabedini project configuration is already in the solver. I've replaced a FilePath with a simple data type that distinguishes between the root and imported FilePath.
data ConstraintSource =
...
- | ConstraintSourceProjectConfig FilePath
+ | ConstraintSourceProjectConfig ProjectConfigPath
I had a quick go at making ConstraintSource polymorphic but so many other things depend on it that this would lead to a lot of cascading changes.
What I can do is move some pretty printing functions from cabal-install-solver to cabal-install with https://github.com/cabalism/cabal/commit/fc87c2919317d634e989d83debae6783c33baf9f, functions showConstraintSource and showProjectConfigPath.
Does that help?
@grayjay would you want me to move those printing functions out of cabal-install-solver?
@philderbeast I know it's already used but we could try to improve the separation between those parts (provided you agree with my assessment, of course).
I spent some time looking at this today and ConstraintSource is barely used in cabal-install-solver. It's only part of FailReason. Adding a type variable for it was indeed a bit invasive since it ends up propagating to Tree, which is used everywhere. But by using existential qualification it seems to be possible to confine the changes to small part of the code. Done that I was able to move ConstraintSource back into cabal-install. I haven't quite finished yet. I'll let you know how it goes.
@andreabedini your changes sound promising. Do you have a WIP branch I could look at?
Do you want to raise a separate issue for improving separation between the solver, ConstraintSource and printing failures?
@philderbeast I opened https://github.com/haskell/cabal/pull/9642 to not block you here.
I've fixed the remaining CI problem (@gbaz using project overrides as they currently work ;-)).
@grayjay could you please take a look at this? @andreabedini has #9642 that will tease ConstraintSource out of cabal-install-solver separately.
This change that shows more context on a constraint conflict (the path to the conflict in the import tree) is ready for review.
While working on this, I uncovered and fixed some bugs with imports. These fixes can be seen in the changes to cabal-testsuite/PackageTests/ConditionalAndImport/cabal.test.hs.
@Mikolaj thanks for expediting #9665 and #9680 ahead of this change. Do you know anyone that could be interested in being second reviewer? I'm keen to do more work in this area after this merges.
@gbaz thanks for proposing project overrides for summer of code 2024.
This pull request has been sitting idle since 1-Feb waiting on a second reviewer. Is anyone available to help?
Ideally @grayjay and/or @gbaz would be so kind as to review this, since it relates to the dark arts of Solving.
Meta
I'm a little worried about how this fits into a bigger picture of improving solver's output. See e.g. #8475. Especially I'm always nervous when we add more information to the default output. In this case, the solver is already infamous for being mouthful. I'd feel much easier if this new output was hidden behind a flag. But I understand that people may have reasons to want it on by default. Such tradeoffs are always hard to settle.
Feedback
It'd be good to show an example output right in the PR description. I'm going with what I see in the changelog file...
[__1] next goal: hashable
[__1] rejecting: hashable-1.4.3.0 (constraint from project requires ==1.4.2.0)
+-- cabal.project requires ==1.4.2.0
[__1] rejecting: hashable-1.4.2.0 (constraint from project requires ==1.4.3.0)
+-- cabal.project
+-- https://www.stackage.org/nightly-2023-12-07/cabal.config requires ==1.4.3.0
I have the following issues.
First, it'd be nice to show "before and after", not just "after", otherwise it's hard to judge presentation, and presentation is important here.
Second, the nested part doesn't seem aligned right to me. I'm taking as an example a variant of tree (exa -T):
❯ exa -T
.
├── Cabal-syntax.cabal
├── ChangeLog.md
├── LICENSE
├── README.md
├── Setup.hs
└── src
├── Distribution
│ ├── Backpack.hs
│ ├── CabalSpecVersion.hs
By analogy, the relevant portion of output should be something like:
+-- cabal.project
+-- https://www.stackage.org/nightly-2023-12-07/cabal.config requires ==1.4.3.0
Note the two extra spaces in the second line.
Third, the relative alignment between [__1] and +-- looks strange to me but I guess it more or less follows the logic of tree I discuss above. So, I'm not sure how to improve it. Perhaps, it's fine.
Unicode
I'm all for it. It'd make things much more readable. And we can create a precedent that others will be happy to follow. I think we shouldn't kick the can down the road once again and should try to settle this question in this PR. This will unlock #8475 and perhaps others.
Could you invest a bit of your time into researching: what exactly may go wrong with Unicode? One thing I'd try is to run cabal with TERM=dumb and see how your output looks. But that's a cheap guess. Perhaps the Internet knows more about what kind of arrangements can be made so that we don't blow up someone's terminal with our emoji and stuff... Maybe there's a nice Haskell package that abstracts some logic to choose between unicode v. ascii based on the environment? Cabal-install(-solver) could use more packages no problem, I think.
https://downloads.haskell.org/ghc/9.2.5/docs/html/libraries/base-4.16.4.0/GHC-IO-Encoding-Iconv.html with https://downloads.haskell.org/ghc/9.2.5/docs/html/libraries/base-4.16.4.0/GHC-IO-Encoding-Failure.html#t:CodingFailureMode may be of interest here.
Third, the relative alignment between
[__1]and+--looks strange to me but I guess it more or less follows the logic oftreeI discuss above. So, I'm not sure how to improve it. Perhaps, it's fine.
This was the first available column that didn't visually seem to encroach on the [__n] step counter. I tried further left alignment and it looked odd.
I only used as much indentation as needed to show nesting but am happy to indent more. It is a quick fix. Some imports are quite long and I wanted to save columns.
... cabal.project
- +-- https://www.stackage.org/nightly-2023-12-07/cabal.config requires ==1.4.3.0
+ +-- https://www.stackage.org/nightly-2023-12-07/cabal.config requires ==1.4.3.0
It'd be good to show an example output right in the PR description.
Thanks for the review @ulysses4ever and thanks for the suggestion. I've done this.
@ulysses4ever I'd really rather not tackle unicode output on this pull request but would be happy to investigate it on a separate issue.
Especially I'm always nervous when we add more information to the default output.
That's a good point @ulysses4ever especially in the case where there's only one cabal.project with no further imports.
I'd really rather not tackle unicode output on this pull request but would be happy to investigate it on a separate issue.
Fair enough. Let's wait up to one more week to see if Mikolaj's pings come through and we get the attention of the solver gurus.
First, it'd be nice to show "before and after", not just "after", otherwise it's hard to judge presentation, and presentation is important here.
Do the tests' .out files show enough detail of before and after?
especially in the case where there's only one cabal.project with no further imports.
Indeed. I'd potentially look into special-casing one project file and see if the output can be compacted in that case. It may look a little ugly in code, but may look much nicer to the user, and the latter is what I care more about.
That's, again, more like food for thought than an actual suggestion.
Do the tests' .out files show enough detail of before and after?
Well, diffs are not always pleasant to read. Also, it'd be interesting to see deeper nesting than what we have in tests. Finally, as a rule of thumb, I suggest to always put before/after for UI/X-related PR into PRs description: it just gives a better chance for getting people to discuss it, in my experience.
I only used as much indentation as needed to show nesting but am happy to indent more. It is a quick fix. Some imports are quite long and I wanted to save columns.
I was wondering if the nested indentation was necessary. Could all of the paths be indented by the same amount, since they form a list, not a tree? I also think that the +-- should be indented at least as far as "rejecting" in the message above.
From Haskell with UTF-8, I was able to print the unicode chars I need for the tree with just these changes;
git diff cabal-install/src/Distribution/Client/Main.hs
diff --git a/cabal-install/src/Distribution/Client/Main.hs b/cabal-install/src/Distribution/Client/Main.hs
index 6ef0a6737..b6408bf52 100644
--- a/cabal-install/src/Distribution/Client/Main.hs
+++ b/cabal-install/src/Distribution/Client/Main.hs
@@ -96,6 +96,8 @@ import Distribution.Simple.Setup
, toFlag
)
+import Main.Utf8 (withUtf8)
import Distribution.Client.Compat.Prelude hiding (get)
import Prelude ()
@@ -299,7 +301,7 @@ import System.Process (createProcess, env, proc)
-- producing 'CommandParse' data, which mainWorker pattern-matches
-- into IO actions for execution.
main :: [String] -> IO ()
-main args = do
+main args = withUtf8 $ do
installTerminationHandler
-- Enable line buffering so that we can get fast feedback even when piped.
-- This is especially important for CI and build systems.
git diff cabal-install-solver/src/Distribution/Solver/Types/ProjectConfigPath.hs
diff --git a/cabal-install-solver/src/Distribution/Solver/Types/ProjectConfigPath.hs b/cabal-install-solver/src/Distribution/Solver/Types/ProjectConfigPath.hs
index 3143276d0..198b2929b 100644
--- a/cabal-install-solver/src/Distribution/Solver/Types/ProjectConfigPath.hs
+++ b/cabal-install-solver/src/Distribution/Solver/Types/ProjectConfigPath.hs
@@ -41,7 +41,7 @@ instance Structured ProjectConfigPath
showProjectConfigPath :: ProjectConfigPath -> String
showProjectConfigPath (ProjectConfigPath xs) =
unlines
- [ (nTimes i (showChar ' ') . showString "+-- " . showString x) ""
+ [ (nTimes i (showChar ' ') . showString "└─ " . showString x) ""
| x <- reverse $ toList xs
| i <- [0..]
]
Is it acceptable for cabal-install to take a dependency on with-utf8?
I double checked
From Haskell with UTF-8, I was able to print the unicode chars I need for the tree with just these changes;
Scratch that optimism :cry: . I think that I must have always been able to print those unicode chars to my shell (fish on ubuntu). It must have been cabal-testsuite:cabal-tests that was not printing the unicode chars correctly. It prints ?? instead of └─ to cabal-testsuite/PackageTests/ConditionalAndImport/cabal.out when I run that test.
I also think that the
+--should be indented at least as far as "rejecting" in the message above.
I've done this @grayjay.
I was wondering if the nested indentation was necessary. Could all of the paths be indented by the same amount, since they form a list, not a tree?
@grayjay I tried the list and it doesn't work for me. I immediately wonder, do all items in the list import the .config with the conflict?
[__1] rejecting: hashable-1.4.3.0
(<ROOT>/3-web-import-constraints.project requires ==1.4.2.0)
[__1] rejecting: hashable-1.4.2.0
(https://www.stackage.org/nightly-2023-12-07/cabal.config requires ==1.4.3.0)
imported by:
- stackage-web.config
- hop-web.config
- <ROOT>/3-web-import-constraints.project
Here are some keyed list alternatives that may be better than a plain list but they feel clunky;
[__1] rejecting: hashable-1.4.2.0
(#4. https://www.stackage.org/nightly-2023-12-07/cabal.config requires ==1.4.3.0)
#4. imported by #3. stackage-web.config
#3. imported by #2. hop-web.config
#2. imported by #1. <ROOT>/3-web-import-constraints.project
[__1] rejecting: hashable-1.4.2.0
(#4. https://www.stackage.org/nightly-2023-12-07/cabal.config requires ==1.4.3.0)
#3. stackage-web.config
#2. hop-web.config
#1. <ROOT>/3-web-import-constraints.project
(#1 imports (#2 imports (#3 imports #4)))
Showing the .config with the conflict first seems better, not so far to scan with the eyes from the "rejecting" to the "requires".
Of the list-like displays I've tried so far, this is the one I like the most and the one I've commited;
[__1] rejecting: hashable-1.4.3.0
(<ROOT>/3-web-import-constraints.project requires ==1.4.2.0)
[__1] rejecting: hashable-1.4.2.0
(https://www.stackage.org/nightly-2023-12-07/cabal.config requires ==1.4.3.0)
imported by: stackage-web.config
imported by: hop-web.config
imported by: <ROOT>/3-web-import-constraints.project
I'd potentially look into special-casing one project file and see if the output can be compacted in that case.
I've done this @grayjay, showProjectConfigPathFailReason prints a single line for one project with no imports or where the failure is in the project root.
@grayjay I've made suggested changes, responded to questions and added another test case that now catches Y-shaped duplicate imports. This pull request is now ready for a follow up review.