Duplicate project import as a warning only by philderbeast · Pull Request #10933 · haskell/cabal

philderbeast · 2025-04-21T15:38:53Z

Depends-on #11773.

I'll squash commits before applying the merge label if this pull request is approved.

Pretty much the same as #9933 but gives a warning instead of an error when duplicate imports that are not cyclical are detected. I did this work in Oct 2024 but didn't raise a pull request for it then. I was reporting the warning during parsing but needed an IORef for that. After Matthew Pickering suggesting it, I'm now doing the reporting after parsing.

Add a new module cabal-install/src/Distribution/Client/ProjectConfig/Import.hs and move some stuff there from cabal-install-solver/src/Distribution/Solver/Types/ProjectConfigPath.hs that was only ever used in cabal-install.
Shorten the haddock sections in Distribution.Client.ProjectConfig.Legacy so that they don't wrap when rendered.

For messages, I added some doctests and ended up with two ways of comparing ProjectConfigPath.

-- | A comparison that puts projects first, URLs last and sorts the other paths
-- lexically.
compareLexically :: ProjectConfigPath -> ProjectConfigPath -> Ordering

-- | A comparison that puts projects first, URLs last and sorts the other paths
-- by putting longer paths after shorter ones as measured by the number of path
-- segments. If still equal, then sorting is lexical.
compareSegmentally:: ProjectConfigPath -> ProjectConfigPath -> Ordering

I changed the project skeleton, retaining the info we have already (because we have to read the file), that the item is a local file or a URI.

- type ProjectConfigSkeleton = CondTree ConfVar ([ProjectConfigPath], ProjectConfig)
+ type ProjectConfigSkeleton = CondTree ConfVar ([(Maybe URI, ProjectConfigPath)], ProjectConfig)

With that information, I'm able to avoid monitoring remote files:

- monitorFiles $ map monitorFileHashed (projectConfigPathRoot <$> projectSkeletonImports pcs)
+ monitorFiles
+  [ monitorFileHashed (projectConfigPathRoot path)
+  | (Nothing, path) <- projectSkeletonImports pcs
+  ]

I added another type to help classify between, root, file import and URI import. I used this to help detect and report duplicates:

-- | Isomorphic with 'ProjectConfigPath' but with separate constructors for the
-- root, imported file and imported URI.
data ProjectNode a where
  ProjectRoot :: FilePath -> ProjectNode ProjectFilePath
  ProjectFileImport :: FilePath -> ProjectConfigPath -> ProjectNode FilePath
  ProjectUriImport :: URI -> ProjectConfigPath -> ProjectNode URI

I developed this with ghc-9.14.1 and required some extra type annotations for prior GHC versions to compile, those in seenImport and instance Ord (ProjectNode a).

I moved the following types and functions to the Import module too. These were either in the Legacy parser or in both parsers.

type ProjectConfigSkeleton = CondTree ConfVar ([(Maybe URI, ProjectConfigPath)], ProjectConfig)

projectSkeletonImports :: ProjectConfigSkeleton -> [(Maybe URI, ProjectConfigPath)]
projectSkeletonImports = fst . view traverseCondTreeA

fetchImportConfig
  :: FilePath
  -> HttpTransport
  -> Verbosity
  -> FilePath
  -> ProjectConfigPath
  -> IO (Maybe URI, BS.ByteString)

Patches conform to the coding conventions.
Any changes that could be relevant to users have been recorded in the changelog.
- Is the change significant? If so, remember to add significance: significant in the changelog file.
The documentation has been updated, if necessary.
Manual QA notes have been included.
Tests have been added. (Ask for help if you don’t know how to write them! Ask for an exemption if tests are too complex for too little coverage!)

ulysses4ever

Thanks!

philderbeast · 2025-08-12T00:47:01Z

I've updated the tests and moved them to cabal-testsuite/PackageTests/ProjectImport/UniquePathDuplicates/cabal.test.hs, fixing them for Windows and for the changes in the project parser.

philderbeast · 2025-08-12T01:55:35Z

This pull request is complete for the --project-file-parser=legacy. I'd prefer to add this feature to the parsec parser on a separate pull request.

mpickering · 2025-08-12T15:42:44Z

Is there an issue which describe the problem or feature this PR is adding?

mpickering · 2025-08-12T15:43:52Z

This pull request is complete for the --project-file-parser=legacy. I'd prefer to add this feature to the parsec parser on a separate pull request.

In the next + 1 release the legacy parser will be removed, so I would advise implementing the feature firstly for the parsec parser.

philderbeast · 2025-08-12T15:58:44Z

Is there an issue which describe the problem or feature this PR is adding?

From the description in this pull request:

Pretty much the same as #9933

From the description of #9933:

A follow on from #9578.

From the description of #9578:

Fixes #9562.

Before I started working on this stuff, there were some tests for cyclical imports but none for duplicates without cycles. This pull request adds these tests and reporting those duplicates as a warning when we'd punted on including this reporting as an error in an earlier pull request in the chain.

ulysses4ever · 2026-05-21T14:15:49Z

- type ProjectConfigSkeleton = CondTree ConfVar ([ProjectConfigPath], ProjectConfig)
+ type ProjectConfigSkeleton = CondTree ConfVar ([(Maybe URI, ProjectConfigPath)], ProjectConfig)

the level of nestedness is a little much here. Would it be possible to have a type synonym (at least) for (Maybe URI, ProjectConfigPath)? Something like ProjectConfigPathWithProvenance or some such.

philderbeast · 2026-05-21T14:39:34Z

- (Maybe URI, ProjectConfigPath)
+ ProjectConfigPathWithProvenance

The suggested type synonym is longer than the tuple and I'm not able to see what it is at a glance. Once we get rid of the legacy parser, there'll be only one type signature using this 2-tuple outside of the Distribution.Client.ProjectConfig.Import module:

cabal/cabal-install/src/Distribution/Client/ProjectConfig/Parsec.hs

Line 200 in c3c060f

    
           sanityWalkBranch :: CondBranch ConfVar ([ProjectConfigPath], ProjectConfig) -> ParseResult ProjectFileSource ()

philderbeast · 2026-05-22T15:56:54Z

@geekosaur and @ulysses4ever thanks for your approvals. I've resolved all open review comments and will add the merge me label when it has passed the check run, unless you have any further objections?

- Add Y-forking import test - A test for detecting when the same config is imported via many different paths - Error on duplicate imports - Do the filtering in duplicateImportMsg - Use duplicateImportMsg for cycles too - Add haddocks to IORef parameter - Add changelog entry - Use ordNub instead of nub - Use NubList - Share implement of duplicate and cyclical messages - Update expectation for non-cyclical duplicate import - Only show a warning - Add woops project with a time cost - Use noticeDoc instead of warn - Render duplicate imports - Add Ord instance for Dupes, sort on dupesNormLocPath - Fixups after rebase - Satisfy hlint - Remove -XMultiWayIf - Remove mention of yops from the changelog - Satisfy fix-whitespace - Test with a time cost of duplicate imports - Fewer imports from PrettyPrint qualified as Disp - Add data ProjectImport replacing tuples - Combine fields as ProjectImport - Rename field to dupesImports - Add haddocks to Dupes fields - Mark test as flaky - Any test accessing stackage seems susceptible - Move unique duplicates to own test - Use legacy parser for path duplicates test - Add foo.cabal package so that packages exist - Satisfy fix-whitespace - Use local version of lts-21.25 - Remove repo - Use </> for expected paths - Note that this change gives a warning. - Exclude lts-21.25.config from typos - Detect duplicate imports after parsing - Merge upstream - Update duplicate import counts - Satisfy fourmolu - Move duplicate detection to calling module - Add parsing module haddock section header - Export only reportDuplicateImports - Test all project parser options - Add script tests - Add haddocks to reportDuplicateImportsh - Use Data.Function ((&)) pipelining - Inline do blockh - Satisfy fix-whitespace - Redo Ord Dupes - Try to enforce sorting - Add docs to ProjectImport - Add instances & doctests for sorting ProjectImport - Fix sorting of ProjectConfigPath - Update test expectation with correct sorting - Compare versions of compare - Update cabal.out for dedup tests - Rename to compareForDisplay - Add more detail to docProjectConfigFiles docs - Add haddocks and rename comparisons - Don't repeat the root comment - Pattern match on NonEmpty not List - Sort expectation by import chain length - Use GADTs for ProjectImport - Satisfy hlint, remove unused LANGUAGE pragmas - Bubble up Maybe URI - Add ProjectRoot constructorh - Rename ProjectImport to ProjectNode - Add Maybe URI to project skeleton - Use ProjectFilePath - Satisfy hlint - Add projectNodes - Redo detectDupes - Move duplicate imports to Import module - Move fetchImportConfig to Import module - Don't export parseProjectSkeleton - Don't re-export ProjectConfigSkeleton - Remove unnecessary primes - Add splitImports - Remove ProjectNodes and projectNodes - Move stuff to Client.ProjectConfig.Import - Fixup after rebase - Satisfy fourmolu - Don't monitor (Nothing :: Maybe URI) - Add type annotation for older ghc versions - Setup for doctests - Need -XConstraintKinds with older ghcsh - Update .out files for UniquePathDuplicates - Don't repeat recordMode - Remove +legacy-comparison - Note significance in changelog and movements - Less indent - Remove redundant comparison - Hide newtype ProjectFilePath - Don't export duplicateImportMsg or ProjectNode - Gut lts-21.25.config but leave notes

philderbeast · 2026-05-25T11:48:02Z

@mergify refresh

mergify · 2026-05-25T11:48:23Z

refresh

✅ Pull request refreshed

philderbeast · 2026-05-25T13:29:43Z

@mergify queue

mergify · 2026-05-25T13:30:19Z

philderbeast · 2026-05-25T17:29:02Z

This pull request was dequeued because of a failing run. I reran that failing run and it succeeded. @Mikolaj, @ulysses4ever, @geekosaur is this something that mergify could do itself, try the failing job once more before dequeuing?

The job that failed and caused the dequeue as reported earlier in this thread:

https://github.com/haskell/cabal/actions/runs/26403491730/job/77734323407

The run that passed when I reran its failed jobs:

https://github.com/haskell/cabal/actions/runs/26403491730

philderbeast · 2026-05-25T18:09:02Z

@mergify queue

mergify · 2026-05-25T18:09:16Z

philderbeast force-pushed the fix/y-fork-project-import-warning-only branch from 16a11bd to 95ec86a Compare April 21, 2025 15:41

geekosaur approved these changes Apr 21, 2025

View reviewed changes

ulysses4ever approved these changes Apr 21, 2025

View reviewed changes

Comment thread cabal-install/src/Distribution/Client/ProjectConfig/Legacy.hs Outdated

Comment thread cabal-install/src/Distribution/Client/ProjectConfig/Legacy.hs Outdated

Comment thread cabal-install/src/Distribution/Client/ProjectConfig/Legacy.hs Outdated

philderbeast force-pushed the fix/y-fork-project-import-warning-only branch 3 times, most recently from c230d20 to b61e031 Compare April 27, 2025 00:52

sebright added the re: project-file Concerning cabal.project files label May 8, 2025

philderbeast force-pushed the fix/y-fork-project-import-warning-only branch from a926d99 to ec9ea13 Compare July 6, 2025 19:58

philderbeast force-pushed the fix/y-fork-project-import-warning-only branch from ec9ea13 to 7ab2b20 Compare July 15, 2025 14:19

philderbeast force-pushed the fix/y-fork-project-import-warning-only branch 2 times, most recently from d5552f3 to 0c99ae5 Compare July 25, 2025 12:25

philderbeast force-pushed the fix/y-fork-project-import-warning-only branch 3 times, most recently from 7fe8c13 to 61d8a74 Compare August 11, 2025 15:10

philderbeast mentioned this pull request Aug 11, 2025

Detect duplicate non-cyclical project imports #9933

Closed

5 tasks

philderbeast force-pushed the fix/y-fork-project-import-warning-only branch 2 times, most recently from fd40a31 to 814b84c Compare August 11, 2025 19:29

philderbeast force-pushed the fix/y-fork-project-import-warning-only branch from 65bba25 to 3d032f3 Compare August 12, 2025 00:51

philderbeast mentioned this pull request Aug 12, 2025

Difference in --project-file-parser=compare #11152

Closed

mpickering reviewed Aug 13, 2025

View reviewed changes

Comment thread cabal-install/src/Distribution/Client/ProjectConfig/Legacy.hs Outdated

philderbeast force-pushed the fix/y-fork-project-import-warning-only branch 2 times, most recently from 3021e83 to bd7c594 Compare August 14, 2025 13:28

philderbeast force-pushed the fix/y-fork-project-import-warning-only branch 2 times, most recently from 557c9b3 to f62f929 Compare August 27, 2025 13:09

philderbeast force-pushed the fix/y-fork-project-import-warning-only branch from f62f929 to 90cafdb Compare March 12, 2026 12:59

philderbeast force-pushed the fix/y-fork-project-import-warning-only branch 8 times, most recently from a6476d4 to c8e778f Compare May 19, 2026 19:49

philderbeast force-pushed the fix/y-fork-project-import-warning-only branch 2 times, most recently from 9a3b1d2 to 743a236 Compare May 22, 2026 15:51

philderbeast force-pushed the fix/y-fork-project-import-warning-only branch from 743a236 to 976109a Compare May 22, 2026 15:58

philderbeast added merge me Tell Mergify Bot to merge and removed attention: needs-review labels May 22, 2026

philderbeast force-pushed the fix/y-fork-project-import-warning-only branch from 976109a to dbb260b Compare May 22, 2026 20:01

mergify Bot added the ready and waiting Mergify is waiting out the cooldown period label May 22, 2026

philderbeast force-pushed the fix/y-fork-project-import-warning-only branch from dbb260b to 246537a Compare May 24, 2026 00:18

mergify Bot mentioned this pull request May 25, 2026

merge queue: embarking master (0a8508e) and #10933 together #11882

Closed

86 tasks

mergify Bot mentioned this pull request May 25, 2026

merge queue: embarking master (0a8508e) and #10933 together #11883

Closed

86 tasks

philderbeast mentioned this pull request May 25, 2026

Monitor imported cabal.project files #11884

Open

10 tasks

Conversation

philderbeast commented Apr 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ulysses4ever left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

philderbeast commented Aug 12, 2025

Uh oh!

philderbeast commented Aug 12, 2025

Uh oh!

mpickering commented Aug 12, 2025

Uh oh!

mpickering commented Aug 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

philderbeast commented Aug 12, 2025

Uh oh!

Uh oh!

ulysses4ever commented May 21, 2026

Uh oh!

philderbeast commented May 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

philderbeast commented May 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

philderbeast commented May 25, 2026

Uh oh!

mergify Bot commented May 25, 2026

✅ Pull request refreshed

Uh oh!

philderbeast commented May 25, 2026

Uh oh!

mergify Bot commented May 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merge Queue Status

Reason

Hint

Uh oh!

philderbeast commented May 25, 2026

Uh oh!

philderbeast commented May 25, 2026

Uh oh!

mergify Bot commented May 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merge Queue Status

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

philderbeast commented Apr 21, 2025 •

edited

Loading

mpickering commented Aug 12, 2025 •

edited

Loading

philderbeast commented May 21, 2026 •

edited

Loading

philderbeast commented May 22, 2026 •

edited

Loading

mergify Bot commented May 25, 2026 •

edited

Loading

mergify Bot commented May 25, 2026 •

edited

Loading