Deduplicate "using config from" message

[philderbeast]

Adds an Ord instance for ProjectConfigPath and fixes the duplication in the reported "Configuration is affected by the following files", fixes #10509 by using docProjectConfigFiles instead of docProjectConfigPath.

Template Α: This PR modifies behaviour or interface

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

[ulysses4ever]

This fixes the duplication in the reported "Configuration is affected by the following files"

I think it fixes only part of the duplication problem in the snippet you reference. I.e. it dedups file names but it doesn't dedup the phrase "Configuration is affected by the following files". The latter seem to be a bug in the way that snippet is written: there shouldn't be a sequence_ at all. What it should look like is:

    notice verbosity . render . vcat $
      text "Configuration is affected by the following files:"
        : [ text "-" <+> docProjectConfigPath path
          | Explicit path <- Set.toList $ projectConfigProvenance projectConfig
          ]

[ulysses4ever]

I'm already doing dedup of the intro message as a part of #10548 btw. It'd be best if this one only handled the Ord instance.

It's also a bit more complicated than I said above: with implicit config provenance, it'll print the intro message and then nothing else, which doesn't make sense. THis is not hard to fix of course.

[philderbeast]

@mpickering for these added tests do we already have a normalization of the project root directory for tests, something like <PROJECT-ROOT>, now that tests are run in temporary directories with #9717?

$ git diff 
...
--- a/cabal-testsuite/PackageTests/ConditionalAndImport/cabal.test.hs
+++ b/cabal-testsuite/PackageTests/ConditionalAndImport/cabal.test.hs
@@ -261,6 +261,49 @@ main = cabalTest . withRepo "repo" . recordMode RecordMarked $ do
   log "checking if we detect when the same config is imported via many different paths (we don't)"
   woopping <- cabal' "v2-build" [ "--project-file=woops-0.project" ]
 
+  log "checking \"using config from message\" without URI imports"
+  withDirectory "yops" $ do
+    yopping <- fails $ cabal' "v2-build" [ "--project-file=../yops-0.project" ]
+    assertOutputContains
+      (normalizeWindowsOutput "When using configuration from: \
+      \ - /tmp/cabal-testsuite-286573/yops-0.project \
+      \ - /tmp/cabal-testsuite-286573/yops-2.config \
+      \ - /tmp/cabal-testsuite-286573/yops-4.config \
+      \ - /tmp/cabal-testsuite-286573/yops-6.config \
+      \ - /tmp/cabal-testsuite-286573/yops-8.config \
+      \ - /tmp/cabal-testsuite-286573/yops/yops-1.config \
+      \ - /tmp/cabal-testsuite-286573/yops/yops-3.config \
+      \ - /tmp/cabal-testsuite-286573/yops/yops-5.config \
+      \ - /tmp/cabal-testsuite-286573/yops/yops-7.config \
+      \ - /tmp/cabal-testsuite-286573/yops/yops-9.config \
+      \ The following errors occurred: \
+      \  - The package directory '.' does not contain any .cabal file.")
+      yopping
+
+    return ()
+
+  log "checking \"using config from message\" with URI imports"
+  withDirectory "woops" $ do
+    woopping <- fails $ cabal' "v2-build" [ "--project-file=../woops-0.project" ]
+    assertOutputContains
+      (normalizeWindowsOutput "When using configuration from: \
+      \ - /tmp/cabal-testsuite-282695/woops-0.project \
+      \ - /tmp/cabal-testsuite-282695/woops-2.config \
+      \ - /tmp/cabal-testsuite-282695/woops-4.config \
+      \ - /tmp/cabal-testsuite-282695/woops-6.config \
+      \ - /tmp/cabal-testsuite-282695/woops-8.config \
+      \ - /tmp/cabal-testsuite-282695/woops/woops-1.config \
+      \ - /tmp/cabal-testsuite-282695/woops/woops-3.config \
+      \ - /tmp/cabal-testsuite-282695/woops/woops-5.config \
+      \ - /tmp/cabal-testsuite-282695/woops/woops-7.config \
+      \ - /tmp/cabal-testsuite-282695/woops/woops-9.config \
+      \ - https://www.stackage.org/lts-21.25/cabal.config \
+      \ The following errors occurred: \
+      \  - The package directory '.' does not contain any .cabal file.")
+      woopping
+
+    return ()

[ulysses4ever]

something like <PROJECT-ROOT>

The previous person who bumped into this problem wasn't able to find an existing solution: #8617 (comment)

[philderbeast]

@ulysses4ever and @mpickering, to get around the /tmp/cabal-testsuite-286573/yops-0.project problem I used assertRegex.

I see that cabal' "v2-build" [ "--project-file=yops-0.project" ] does not set the test environment's project.

cabal/cabal-testsuite/src/Test/Cabal/Monad.hs

Lines 790 to 791 in 7de199a

	-- | Says what cabal.project file to use (probed)
	, testCabalProjectFile :: Maybe FilePath

[philderbeast]

This will need another review because previously I'd mucked up the Ord instance.

[philderbeast]

I didn't add a test of an http import importing another http config file. Two possible ways of doing this are running a local server for the test or picking up the import from somewhere like https://raw.githubusercontent.com/haskell/cabal/refs/heads/master/cabal.project.

[ulysses4ever]

I'm traveling this week, so will only be able to get to this next week at earliest. Sorry!

[philderbeast]

I'm already doing dedup of the intro message

@ulysses4ever there are two similar messages. You've dealt with the Configuration is affected by the following files one in #10548 and I'm dealing with the other one here, with When using configuration from.

$ grep -R -E 'When using configuration from|Configuration is' ./**/*.hs --exclude='*.test.hs'
./cabal-install/src/Distribution/Client/ProjectConfig.hs:      "When using configuration from:\n"
./cabal-install/src/Distribution/Client/ProjectPlanning.hs:        text "Configuration is affected by the following files:" : configfiles

[geekosaur]

Depends on whether you want a review of the current code or not. As I just noted, Artem's approval was of a very early version.

[philderbeast]

I guess we're in a kind of limbo, while @ulysses4ever has a pending re-review, I can neither request another review or invalidate his prior approval.

You're right @geekosaur, you're the only one to have reviewed the later changes since I fixed the Ord instance.

[geekosaur]

I can dismiss it (click the downarrow to the right of "2 approvals", then the 3-dots to the right of his approval), but I'm not sure if that's an admin action or something you can do. Do you want me to dismiss it?

[philderbeast]

Thanks @geekosaur, I can dismiss it.

This was a review of an earlier version with an incorrect Ord instance.

[Mikolaj]

This is quite delicate and I guess the devil might be in the details, but the tests are solid and LGTM.

[philderbeast]

@Mikolaj, I've updated the haddocks on Ord ProjectConfigPath.

[philderbeast]

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

[9999years]

Looks reasonable. I think the release note could use a before/after example to show how this helps users. (I don't care about an Ord instance but I care a lot about error message quality!)

[philderbeast]

@9999years I've updated the changelog entry to talk more about the "using configuration from" message changes.

@Kleidukos what is the attention: needs-manual-qa label for other than to hold off on mergify merging? If that is its purpose then I can wait until @9999years gives me the OK on the changelog, squash the recent commits and then remove this label myself.

[Kleidukos]

@philderbeast that label does not prevent merging, it's for after-merge :)

[9999years]

(I had also approved it beforehand so as not to block the merge)