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

Show import tree provenance #9578

Merged
merged 1 commit into from
Apr 6, 2024

Conversation

philderbeast
Copy link
Collaborator

@philderbeast philderbeast commented Dec 31, 2023

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.8 and cabal-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 the info level 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 on directory in cabal-install-solver. I also needed network-uri to 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 via https://... doesn't itself import another config. Those extra dependencies we should be able to remove when the suggested change of @andreabedini lands, #9642.

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

@philderbeast philderbeast marked this pull request as draft December 31, 2023 14:00
@philderbeast philderbeast force-pushed the fix/solver-msg-import-tree-9562 branch 4 times, most recently from 706e8e1 to 4c73bfc Compare December 31, 2023 14:30
@philderbeast philderbeast marked this pull request as ready for review December 31, 2023 20:33
@philderbeast philderbeast force-pushed the fix/solver-msg-import-tree-9562 branch 3 times, most recently from 9235104 to 0dd67f8 Compare January 8, 2024 21:13
@philderbeast philderbeast force-pushed the fix/solver-msg-import-tree-9562 branch from 0dd67f8 to 4d6a259 Compare January 16, 2024 18:28
Copy link
Collaborator

@andreabedini andreabedini left a comment

Choose a reason for hiding this comment

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

I think it would be better if we could keep the concept of project configuration outside of the solver, for the sake of modularity and cleaner separation between cabal-install and cabal-install-solver.

I haven't thought about how to do this technically. Perhaps ConstraintSource could be made more polymorphic? In the end, the solver only needs to know whether the constraint comes internally (i.e. from a previous choice during the solving process) or externally.

@philderbeast philderbeast force-pushed the fix/solver-msg-import-tree-9562 branch from 4d6a259 to 6046b12 Compare January 17, 2024 13:51
@philderbeast
Copy link
Collaborator Author

@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 cabalism@fc87c29, functions showConstraintSource and showProjectConfigPath.

Does that help?

@grayjay would you want me to move those printing functions out of cabal-install-solver?

@philderbeast philderbeast force-pushed the fix/solver-msg-import-tree-9562 branch from 6046b12 to 89c7163 Compare January 17, 2024 21:59
@andreabedini
Copy link
Collaborator

@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.

@philderbeast
Copy link
Collaborator Author

@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?

@andreabedini
Copy link
Collaborator

@philderbeast I opened #9642 to not block you here.

@philderbeast philderbeast force-pushed the fix/solver-msg-import-tree-9562 branch from 89c7163 to e7fad63 Compare January 22, 2024 13:26
@philderbeast
Copy link
Collaborator Author

I've fixed the remaining CI problem (@gbaz using project overrides as they currently work ;-)).

@philderbeast
Copy link
Collaborator Author

@grayjay could you please take a look at this? @andreabedini has #9642 that will tease ConstraintSource out of cabal-install-solver separately.

@philderbeast philderbeast force-pushed the fix/solver-msg-import-tree-9562 branch from 5c870a8 to a2d9402 Compare January 23, 2024 21:26
@philderbeast philderbeast force-pushed the fix/solver-msg-import-tree-9562 branch from a2d9402 to 6c0a613 Compare January 25, 2024 12:54
Copy link
Collaborator

@grayjay grayjay left a comment

Choose a reason for hiding this comment

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

Thanks @philderbeast!

@philderbeast
Copy link
Collaborator Author

Label merge+no rebase is necessary when the pull request is from an organisation.

@philderbeast philderbeast force-pushed the fix/solver-msg-import-tree-9562 branch 4 times, most recently from f71c891 to 442f209 Compare April 2, 2024 11:29
@Mikolaj
Copy link
Member

Mikolaj commented Apr 4, 2024

Splendid joint work @philderbeast and @grayjay, in particular, in managing the scope of the PR. Agile at its best, though to make it less stressful, ideally a bit of this work could be moved to the planning/design phase in the issue ticket next time.

I think it needs a manual conflict resolution before it can get merged (a change must have crept in during the 2-day merge delay period).

With this change to the solver message rendering, I also fix some bugs around project imports, adding tests for those cases. Reviewers asked that the Y-shaped import checks (using IORef) be made on a separate pull request. Removing those lead to cascading deletions.

- Regenerate expected .out files
- Show tree provenance of import constraint
- Add trimmed down PackageTests/VersionPriority
- Add changelog entry
- Use NonEmpty
- Fix check for cyclical import
- Use primes for next iteration
- Remove unused LANGUAGE pragmas
- Rename to projectConfigPathRoot
- Docs for ProjectConfigPath and showProjectConfigPath
- Renaming
- Add cyclical import tests with 1 and 2 hops in cycle
- Use full path for cyclical error message
- Expected output has project with full project path
- Add fullPath local function
- Project directory as FilePath, not Maybe FilePath
- Use (_, projectFileName) binding splitFileName
- Need full path to project parsing legacy
- Inline seenImports conversion
- Add cyclical checks with same file names and hops
- Add noncyclical tests that hop over folders
- Add a project testing skipping in and out of a folder
- Update expectations of cyclical tests
- Use canonicalizePath for collapsing .. when possible
- Capture trace for later
- Add module for ProjectConfigPath
- Move functions for ProjectConfigPath to its module
- Fetch URI is not prefixed with ./https://etc
- Document normaliseConfigPath
- Add doctests for normaliseConfigPath
- Add doctest of canonicalizeConfigPath
- Show an example of canonical paths
- Use importer and importee in canonicalizeConfigPaths
- Add logging
- Use normLocPath, drop -XMultiWayIf
- Remove seenImports parameter from parseProject
- Follow hlint suggestion: Move brackets to avoid $
- Follow hlint warning: Use mapM
- Follow hlint warning: Use traverse_
- Follow hlint suggestion: Use <$>
- Add failing test skipping across folder
- Remove normaliseConfigPath
- Add hasDuplicatesConfigPath
- Remove lengthConfigPath and nubConfigPath
- Mention cabal-install-solver in the changelog
- Left align project import tree with other content
- Rerun tests to generate left-aligned tree output
- Don't show "constraint from project requires ..."
- Regenerate test outputs without (constraint from project ...)
- Move fail reason printing to project config path module
- Minimize diff of showFR
- Show an "imported by: " list instead of a tree
- Update info logging, avoiding leading spaces
- When logging I found leading spaces were trimmed
- Don't log relative location and path
- Use unicode tree symbol in haddocks
- A duplicate importing tests
- Add Y-forking import test
- A test for detecting when the same config is imported via many different paths
- Fix a typo with ../noncyclical-same-filename-b.config
- Add an intercepting cabal-testsuite/cabal.project
- Use IORef to detect seen imports
- Get rid of seenImports
- Use IORef [FilePath] of uniqueImports
- Add consProjectConfigPath
- Expand the haddocks of newtype ProjectConfigPath
- Clarify, it's the project root directory
- Rename a function to makeRelativeConfigPath
- Fix whitespace
- Update output snippet in changelog
- Put the info logging together
- Split cycles and duplicates detection
- Add fixes and duplicate import to the change log
- Better reporting of duplicate imports
- Report cycles and duplicates using unique file name
- This is after canonicalizeConfigPath, relative to the project directory
- Get rid of fullLocPath, use normLocPath only
- Do reporting using paths relative to the directory of the project
- Issue a warning for Y-shaped duplicate imports
- Add duplicateImportMsg
- Use pretty printing for duplicate import message
- Use pretty printing for all ProjectConfigPath printing
- Drop a parameter from duplicateImportMsg
- Correct haddocks on docProjectConfigPath
- Satisfy fourmolu
- Remove the surplus space in "imported by "
- Log project parsing at the debug level, up from info
- Simplify the changelog snippet
- Use normalized paths for solver messages
- Satisfy fourmolu
- Relative to the directory of the project
- Duplicate imports are reported as warnings
- No need to mention v2-build
- Remove IORef
- Subdue a check for duplicate imports
- TODO: Catch duplicates across different import paths. We know how to do this
- Remove check for duplicate imports
- Improve the canonicalizeConfigPath doctests
- Add haddocks, drop configPath from go
- Make importsBy NonEmpty
- Use testDir in doctests
- Use canonical test directory
- Regenerate expected outputs
- Assume head of the path is the duplicate
- Get rid of duplicateImportMsg
- Remove unused imports in ProjectConfigPath
- Follow hlint suggestion: use fewer imports
- Remove fullConfigPathRoot
- Follow hlint suggestion: redundant bracket
- Use the original "source" variable name
- Put the source next to its contents
- Go with the original "seenImports" variable name
- Avoid -XMultiWayIf
- Not needed since we aren't checking for duplicates right now
- Be consistent in naming sources
- Move project dir alongside related params
- Rename to pathRequiresVersion
- Add back "constraint from"
- Talk only of cycles and not duplicates
- Don't report line number for cyclical import
- Remove 2nd example in changelog
- Remove Y-forked duplicate import test
- Add "imported by" to assertOutputContains
- Cull excess doctests of canonicalizeConfigPath
- Add deep path doctest
- Update haddocks of canonicalizeConfigPath
- remove idempotent speculation (it isn't idempotent)
- use "indirection" to describe "." and ".."
- makes the path relative to the given directory
- Remove seenImports
- Use Explicit ProjectConfigPath
- Missed one "constraint from" in changelog example
- Don't check (verbosity >= verbose) separately
- While the same level of verbosity as info, it is clearer to not check the level separately.
- Might calculate the path but not use it.
- Add AbsoluteDir
- Keep provenance normalised for display
- Remove stale REVIEW comment
- Storing provenance normalized avoids this
- Fix whitespace
- Satisfy HLint warning: Redundant flip
- Gut the repo/hashable packages
- Revert AbsoluteDir to FilePath
@philderbeast philderbeast force-pushed the fix/solver-msg-import-tree-9562 branch from 442f209 to f8cd563 Compare April 4, 2024 11:02
@mergify mergify bot added the merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days label Apr 6, 2024
@mergify mergify bot merged commit 400a589 into haskell:master Apr 6, 2024
54 checks passed
@ffaf1
Copy link
Collaborator

ffaf1 commented Apr 9, 2024

@mergify backport 3.12

Copy link
Contributor

mergify bot commented Apr 9, 2024

backport 3.12

✅ Backports have been created

mergify bot added a commit that referenced this pull request Apr 9, 2024
Show import tree provenance (backport #9578)
andreabedini added a commit to andreabedini/cabal that referenced this pull request Jun 27, 2024
Braching off the discussion at haskell#9578 (comment)

The solver should not know anything about project configuration as it is
only a cabal-install concept.  The caller should instead be able to add
arbitrary annotation to the global constraints it passes to the solver.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cabal-install: solver merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days merge+no rebase
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Show the import path when reporting solver rejections from project constraints?
8 participants