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

Deduplicate "using config from" message #10546

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

philderbeast
Copy link
Collaborator

@philderbeast philderbeast commented Nov 11, 2024

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:

@philderbeast philderbeast changed the title Fix/dedup using config from Deduplicate "using config from" message Nov 12, 2024
@philderbeast philderbeast force-pushed the fix/dedup-using-config-from branch 2 times, most recently from 290bd75 to 8423986 Compare November 12, 2024 01:21
@ulysses4ever
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Collaborator Author

@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
Copy link
Collaborator

something like <PROJECT-ROOT>

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

@philderbeast philderbeast force-pushed the fix/dedup-using-config-from branch 3 times, most recently from bf674e3 to 7070d18 Compare November 13, 2024 19:43
@philderbeast
Copy link
Collaborator Author

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

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

@philderbeast philderbeast marked this pull request as ready for review November 13, 2024 19:48
@philderbeast
Copy link
Collaborator Author

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

@philderbeast
Copy link
Collaborator Author

philderbeast commented Nov 13, 2024

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
Copy link
Collaborator

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

@philderbeast
Copy link
Collaborator Author

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

@philderbeast philderbeast force-pushed the fix/dedup-using-config-from branch 3 times, most recently from a565c1e to b364cd8 Compare November 17, 2024 13:21
@geekosaur
Copy link
Collaborator

The CI rework (#10503) speeds CI up enormously, fwiw.

@geekosaur
Copy link
Collaborator

I am tempted to say it's the test that was wrong (and recorded as such): should it really have been up to date on creation? That said, it only happening on Windows is worrisome. (But I'm shortly leaving for another doctor appointment, and in any case I can't test on Windows here.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

What should we report when there are duplicate imports?
3 participants