From bfda13aa29e1464483c028ab3475f2d194e6e03e Mon Sep 17 00:00:00 2001 From: Phil de Joux Date: Mon, 23 Dec 2024 09:16:05 -0500 Subject: [PATCH] Rename the changelog with *.md extension - Add a section on cabal-testsuite changes - Rename the function to readFileVerbatim --- .../ConditionalAndImport/cabal.test.hs | 6 +- .../DedupUsingConfigFromComplex/cabal.test.hs | 2 +- .../ParseErrorProvenance/cabal.test.hs | 2 +- cabal-testsuite/src/Test/Cabal/Prelude.hs | 4 +- changelog.d/pr-10646 | 34 --- changelog.d/pr-10646.md | 209 ++++++++++++++++++ 6 files changed, 216 insertions(+), 41 deletions(-) delete mode 100644 changelog.d/pr-10646 create mode 100644 changelog.d/pr-10646.md diff --git a/cabal-testsuite/PackageTests/ConditionalAndImport/cabal.test.hs b/cabal-testsuite/PackageTests/ConditionalAndImport/cabal.test.hs index c322d5816e9..681865df2e5 100644 --- a/cabal-testsuite/PackageTests/ConditionalAndImport/cabal.test.hs +++ b/cabal-testsuite/PackageTests/ConditionalAndImport/cabal.test.hs @@ -112,7 +112,7 @@ main = cabalTest . withRepo "repo" . recordMode RecordMarked $ do log "checking that imports work skipping into a subfolder and then back out again and again" hopping <- cabal' "v2-build" [ "--project-file=hops-0.project" ] - readVerbatimFile "hops.expect.txt" >>= + readFileVerbatim "hops.expect.txt" >>= flip (assertOn multilineNeedleHaystack) hopping . normalizePathSeparators -- The project is named oops as it is like hops but has conflicting constraints. @@ -129,7 +129,7 @@ main = cabalTest . withRepo "repo" . recordMode RecordMarked $ do log "checking conflicting constraints skipping into a subfolder and then back out again and again" oopsing <- fails $ cabal' "v2-build" [ "all", "--project-file=oops-0.project" ] - readVerbatimFile "oops.expect.txt" + readFileVerbatim "oops.expect.txt" >>= flip (assertOn multilineNeedleHaystack) oopsing . normalizePathSeparators -- The project is named yops as it is like hops but with y's for forks. @@ -172,7 +172,7 @@ main = cabalTest . withRepo "repo" . recordMode RecordMarked $ do log "checking that missing package message lists configuration provenance" missing <- fails $ cabal' "v2-build" [ "--project-file=cabal-missing-package.project" ] - readVerbatimFile "cabal-missing-package.expect.txt" + readFileVerbatim "cabal-missing-package.expect.txt" >>= flip (assertOn multilineNeedleHaystack) missing . normalizePathSeparators return () diff --git a/cabal-testsuite/PackageTests/ProjectImport/DedupUsingConfigFromComplex/cabal.test.hs b/cabal-testsuite/PackageTests/ProjectImport/DedupUsingConfigFromComplex/cabal.test.hs index e3820e7e4f7..e1419fb2467 100644 --- a/cabal-testsuite/PackageTests/ProjectImport/DedupUsingConfigFromComplex/cabal.test.hs +++ b/cabal-testsuite/PackageTests/ProjectImport/DedupUsingConfigFromComplex/cabal.test.hs @@ -10,7 +10,7 @@ main = cabalTest . recordMode RecordMarked $ do log "check project configuration with URI imports is listed in full and" log "check package directories and locations are reported in order" - readVerbatimFile "errors.expect.txt" + readFileVerbatim "errors.expect.txt" >>= flip (assertOn multilineNeedleHaystack) out . normalizePathSeparators return () diff --git a/cabal-testsuite/PackageTests/ProjectImport/ParseErrorProvenance/cabal.test.hs b/cabal-testsuite/PackageTests/ProjectImport/ParseErrorProvenance/cabal.test.hs index 495adade1d6..49360b59872 100644 --- a/cabal-testsuite/PackageTests/ProjectImport/ParseErrorProvenance/cabal.test.hs +++ b/cabal-testsuite/PackageTests/ProjectImport/ParseErrorProvenance/cabal.test.hs @@ -6,7 +6,7 @@ main = cabalTest . recordMode RecordMarked $ do outElse <- fails $ cabal' "v2-build" [ "all", "--dry-run", "--project-file=else.project" ] - msg <- readVerbatimFile "msg.expect.txt" + msg <- readFileVerbatim "msg.expect.txt" let msgSingle = lineBreaksToSpaces msg log "Multiline string marking:" diff --git a/cabal-testsuite/src/Test/Cabal/Prelude.hs b/cabal-testsuite/src/Test/Cabal/Prelude.hs index cfa2b22cde5..43688d6f171 100644 --- a/cabal-testsuite/src/Test/Cabal/Prelude.hs +++ b/cabal-testsuite/src/Test/Cabal/Prelude.hs @@ -1292,8 +1292,8 @@ findDependencyInStore pkgName = do -- > "\\\\?\\C:\\Users\\\\AppData\\Local\\Temp\\cabal-testsuite-8376\\errors.expect.txt": -- > permission denied (The process cannot access the file because it is being -- > used by another process.) -readVerbatimFile :: FilePath -> TestM String -readVerbatimFile filename = do +readFileVerbatim :: FilePath -> TestM String +readFileVerbatim filename = do testDir <- testCurrentDir <$> getTestEnv s <- liftIO . readFile $ testDir filename length s `seq` return s diff --git a/changelog.d/pr-10646 b/changelog.d/pr-10646 deleted file mode 100644 index ab1366afaa9..00000000000 --- a/changelog.d/pr-10646 +++ /dev/null @@ -1,34 +0,0 @@ ---- -synopsis: Deduplicate path separator duplicates -packages: [cabal-install-solver] -prs: 10646 -issues: 10645 ---- - -The "using configuration from" message no longer has duplicates on Windows when -the `cabal.project` uses forward slashes for its imports but the message reports -imports with backslashes. - -```diff -$ cat cabal.project -import: dir-a/b.config - -$ cabal build all --dry-run -... -When using configuration from: -- - dir-a/b.config - - dir-a\b.config - - cabal.project -``` - -## Ord ProjectConfigPath Instance Changes - -For comparison purposes, path separators are normalized to the @buildOS@ -platform's path separator. - -```haskell --- >>> let abFwd = ProjectConfigPath $ "a/b.config" :| [] --- >>> let abBwd = ProjectConfigPath $ "a\\b.config" :| [] --- >>> compare abFwd abBwd --- EQ -``` diff --git a/changelog.d/pr-10646.md b/changelog.d/pr-10646.md new file mode 100644 index 00000000000..2d701005a74 --- /dev/null +++ b/changelog.d/pr-10646.md @@ -0,0 +1,209 @@ +--- +synopsis: Configuration messages without duplicates +packages: [cabal-install-solver] +prs: 10646 +issues: 10645 +--- + +The "using configuration from" message no longer has duplicates on Windows when +a `cabal.project` uses forward slashes for its imports but the message reports +the same import again with backslashes. + +```diff +$ cat cabal.project +import: dir-a/b.config + +$ cabal build all --dry-run +... +When using configuration from: +- - dir-a/b.config + - dir-a\b.config + - cabal.project +``` + +## Changed `Ord ProjectConfigPath` Instance + +For comparison purposes, path separators are normalized to the `buildOS` +platform's path separator. + +```haskell +-- >>> let abFwd = ProjectConfigPath $ "a/b.config" :| [] +-- >>> let abBwd = ProjectConfigPath $ "a\\b.config" :| [] +-- >>> compare abFwd abBwd +-- EQ +``` + +## Changes in `cabal-testsuite` + +### Reading Expected Multiline Strings Verbatim + +With `ghc-9.12.1` adding `-XMultilineStrings`, writing multiline string +expectations for `cabal-testsuite/PackageTests/**/*.test.hs` test scripts might +be have been easier but for a catch. We run these tests with older `GHC` +versions so would need to use `-XCPP` for those versions and the C preprocessor +does not play nicely with string gaps. While it is possible to encode a +multiline string as a single line with in embedded LF characters or by breaking +the line up arbitrarily and using `++` concatenation or by calling unlines on a +list of lines, string gaps are the multiline strings of Haskell prior to +`-XMultilineStrings`. + +To avoid these problems and for the convenience of pasting the expected value +verbatim into a file, `readFileVerbatim` can read the expected multiline output +for tests from a text file. This has the same implementation as `readFile` from +the `strict-io` package to avoid problems at cleanup. + +``` +Warning: Windows file locking hack: hit the retry limit 3 while trying to remove +C:\Users\\AppData\Local\Temp\cabal-testsuite-8376 +cabal.test.hs: +C:\Users\\AppData\Local\Temp\cabal-testsuite-8376\errors.expect.txt: removePathForcibly:DeleteFile +"\\\\?\\C:\\Users\\\\AppData\\Local\\Temp\\cabal-testsuite-8376\\errors.expect.txt": +permission denied (The process cannot access the file because it is being used by another process.) +``` + +The other process accessing the file is `C:\WINDOWS\System32\svchost.exe` +running a `QueryDirectory` event and this problem only occurs when the test +fails. + +### Hidden Actual Value Modification + +The `assertOutputContains` function was modifying the actual value (the test +output) with `concatOutput` before checking if it contained the expected value. +This function, now renamed as `lineBreaksToSpaces`, would remove CR values and +convert LF values to spaces. + +```haskell +-- | Replace line breaks with spaces, correctly handling @"\\r\\n"@. +-- +-- >>> lineBreaksToSpaces "foo\nbar\r\nbaz" +-- "foo bar baz" +-- +-- >>> lineBreaksToSpaces "foo\nbar\r\nbaz\n" +-- "foo bar baz" +-- +-- >>> lineBreaksToSpaces "\nfoo\nbar\r\nbaz\n" +-- " foo bar baz" +lineBreaksToSpaces :: String -> String +``` + +With this set up, false positives were possible. An expected value using string +gaps and spaces would match a `concatOutput` modified actual value of +"foo_bar_baz", where '_' was any of space, LF or CRLF in the unmodified actual +value. The latter two are false positive matches. + +```haskell +let expect = "foo \ + \bar \ + \baz" +``` + +False negatives were also possible. An expected value set up using string gaps +with LF characters or with `-XMultilineStrings` wouldn't match an actual value +of "foo_bar_baz", where '_' was either LF or CRLF because these characters had +been replaced by spaces in the actual value, modified before the comparison. + +```haskell +let expect = "foo\n\ + \bar\n\ + \baz" +``` + +```haskell +{-# LANGUAGE MultilineStrings #-} + +let expect = """ + foo + bar + baz + """ +``` + +We had these problems: + +1. The actual value was changed before comparison and this change was not visible. +2. The expected value was not changed in the same way as the actual value. This + made it possible for equal values to become unequal (false negatives) and for + unequal values to become equal (false positives). + +### Explicit Changes and Visible Line Delimiters + +To fix these problems, an added `assertOn` function takes a `NeedleHaystack` +configuration for how the search is made, what to expect (to find the expected +value or not) and how to display the expected and actual values. + +A pilcrow ΒΆ is often used to visibly display line endings but our terminal +output is restricted to ASCII so lines are delimited between `^` and `$` +markers. The needle (the expected output fragment) is shown annotated this way +and the haystack (the actual output) can optionally be shown this way too. + +We can now implement `assertOutputContains` by calling `assertOn`: + +```diff + assertOutputContains :: MonadIO m => WithCallStack (String -> Result -> m ()) +- assertOutputContains needle result = +- withFrozenCallStack $ +- unless (needle `isInfixOf` (concatOutput output)) $ +- assertFailure $ " expected: " ++ needle +- where output = resultOutput result ++ assertOutputContains = assertOn ++ needleHaystack ++ {txHaystack = ++ TxContains ++ { txBwd = delimitLines ++ , txFwd = encodeLf ++ } ++ } +``` + +This is still a lenient match, allowing LF to match CRLF, but `encodeLf` doesn't +replace LF with spaces like `concatOutput` (`lineBreaksToSpaces`) did: + +```haskell +-- | Replace line CRLF line breaks with LF line breaks. +-- +-- >>> encodeLf "foo\nbar\r\nbaz" +-- "foo\nbar\nbaz" +-- +-- >>> encodeLf "foo\nbar\r\nbaz\n" +-- "foo\nbar\nbaz\n" +-- +-- >>> encodeLf "\nfoo\nbar\r\nbaz\n" +-- "\nfoo\nbar\nbaz\n" +-- +-- >>> encodeLf "\n\n\n" +-- "\n\n\n" +encodeLf :: String -> String +``` + +If you choose to display the actual value by setting +`NeedleHaystack{displayHaystack = True}` then its lines will be delimited. + +```haskell +-- | Mark lines with visible delimiters, @^@ at the start and @$@ at the end. +-- +-- >>> delimitLines "" +-- "^$" +-- +-- >>> delimitLines "\n" +-- "^$\n" +-- +-- >>> delimitLines "\n\n" +-- "^$\n^$\n" +-- +-- >>> delimitLines "\n\n\n" +-- "^$\n^$\n^$\n" +-- +-- >>> delimitLines $ encodeLf "foo\nbar\r\nbaz" +-- "^foo$\n^bar$\n^baz$" +-- +-- >>> delimitLines $ encodeLf "foo\nbar\r\nbaz\n" +-- "^foo$\n^bar$\n^baz$\n" +-- +-- >>> delimitLines $ encodeLf "\nfoo\nbar\r\nbaz\n" +-- "^$\n^foo$\n^bar$\n^baz$\n" +delimitLines:: String -> String +``` + +With `assertOn`, supplying string transformation to both the needle and haystack +before comparison and before display can help find out why an expected value is +or isn't found in the test output.