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

"configuration changed" is way too vague #10617

Open
9999years opened this issue Dec 6, 2024 · 8 comments
Open

"configuration changed" is way too vague #10617

9999years opened this issue Dec 6, 2024 · 8 comments
Labels

Comments

@9999years
Copy link
Collaborator

9999years commented Dec 6, 2024

In order, the following will be built:
 - my-0.1 (lib:my) (configuration changed)

What do you mean the configuration changed? How did the configuration change?

Notes

I've investigated this problem a bit. Here are my notes; they'll be useful to anyone interested in fixing this. Producing a diff of the ElaboratedConfiguredPackages (and keeping the code to do so up-to-date when that record gets a new field) is the hard part.

"configuration changed" comes from showBuildStatus in cabal-install/src/Distribution/Client/ProjectOrchestration.hs.

Ultimately it comes when the ElaboratedConfiguredPackage changes. ElaboratedConfiguredPackage is defined in cabal-install/src/Distribution/Client/ProjectPlanning/Types.hs.

Equality of file monitors determined in newPackageFileMonitor in cabal-install/src/Distribution/Client/ProjectBuilding/PackageFileMonitor.hs.

FileMonitor's fileMonitorKeyValid field should return a Maybe ChangedReason or something like that. (I'm making up ChangedReason, could be a String, could be a structured diff...)

IMO the tricky part is keeping the diff display code up-to-date when someone adds or changes a field to ElaboratedConfiguredPackage (or any type contained in ElaboratedConfiguredPackage!).

We don't have tree-diff in scope RN (maybe we could vendor some minimal subset of it?), so we can't automatically generate a diff

@geekosaur
Copy link
Collaborator

The ghost of #9476 walks again!

@philderbeast
Copy link
Collaborator

This is the error I'm seeing in a CI run, https://github.com/haskell/cabal/actions/runs/12138982850/job/33845593860?pr=10546, that I'm not seeing when I run the same test locally (on Windows):

Actual output differs from expected:

stderr:
--- "D:\\a\\_temp\\cabal-testsuite-9832\\cabal.dist\\cabal.out.normalized"	2024-12-03 12:34:26.194758500 +0000
+++ "D:\\a\\_temp\\cabal-testsuite-9832\\cabal.dist\\cabal.comp.out.normalized"	2024-12-03 12:34:26.193763000 +0000
@@ -651,7 +651,12 @@
     imported by: woops/woops-1.config
     imported by: woops-0.project
 Resolving dependencies...
-Up to date
+Build profile: -w ghc-<GHCVER> -O1
+In order, the following will be built:
+ - my-0.1 (lib:my) (configuration changed)
+Configuring my-0.1...
+Preprocessing library for my-0.1...
+Building library for my-0.1...
 # checking "using config from message" without URI imports
 # cabal v2-build
 # checking "using config from message" with URI imports

*** unexpected failure for PackageTests\ConditionalAndImport\cabal.test.hs

@geekosaur
Copy link
Collaborator

And that's pretty much what I was thinking of; I've seen it in CI as well.

@philderbeast
Copy link
Collaborator

philderbeast commented Dec 9, 2024

@geekosaur, I was seeing this with #10546. I was using the same project twice in cabal-testsuite/PackageTests/ConditionalAndImport/cabal.test.hs. Using the project only once avoids the problem.

I was accessing the same project from two different directories:

  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\" with URI imports"
  withDirectory "woops" $ do
    woopping <- fails $ cabal' "v2-build" [ "--dry-run", "--project-file=../woops-0.project" ]

    -- Use assertRegex when the output is tainted by the temp directory, like
    -- this:
    --
    --   When using configuration from:
    --   - /tmp/cabal-testsuite-282695/woops-0.project
    --   - /tmp/cabal-testsuite-282695/woops-2.config etc
    assertRegex
      "Project configuration with URI imports is listed in full"
      "When using configuration from:(\n|\r\n) \
        \ .*woops-0\\.project(\n|\r\n) \
        \ .*with-ghc\\.config(\n|\r\n) \
        \ .*woops-2\\.config(\n|\r\n) \
        \ .*woops-4\\.config(\n|\r\n) \
        \ .*woops-6\\.config(\n|\r\n) \
        \ .*woops-8\\.config(\n|\r\n) \
        \ .*woops-1\\.config(\n|\r\n) \
        \ .*woops-3\\.config(\n|\r\n) \
        \ .*woops-5\\.config(\n|\r\n) \
        \ .*woops-7\\.config(\n|\r\n) \
        \ .*woops-9\\.config(\n|\r\n) \
        \ .*https://www.stackage.org/lts-21.25/cabal.config(\n|\r\n)"
      woopping

    assertOutputContains
      "The following errors occurred: \
      \  - The package directory '.' does not contain any .cabal file."
      woopping

    return ()

@geekosaur
Copy link
Collaborator

Thing is, I occasionally see it with tests that weren't changed by the PR, and generally on only one platform.

@9999years
Copy link
Collaborator Author

@philderbeast For an example spurious 'configuration changed', see: #10524 (comment).

Excerpt (NB this is about code in #10524, not code on master):

The package configuration now includes the project file path:

ElaboratedConfiguredPackage
    { elabUnitId = UnitId "my-0.1-inplace"
    -- ...
    , elabPkgSourceLocation = WithConstraintSource
        { constraintInner = LocalUnpackedPackage "<ROOT>/."
        , constraintSource = ConstraintSourceProjectConfig
            ( ProjectConfigPath
                ( "noncyclical-same-filename-a.project" :| [] )
            )
        }
    -- ...
    }

Therefore, when we rebuild with a different --project-file, it changes the configuration.

@philderbeast
Copy link
Collaborator

Thanks @9999years. Putting related tests in the same folder using projects seems risky.

@9999years
Copy link
Collaborator Author

So this happens when the full, expanded configuration changes. This could mean a lot of things! Here's an easy solution: we write an ediff of the old and new values to a file under dist-newstyle and print the path to that file so anyone interested in debugging "configuration changed" can cat it.

Or we could just write a "describe configured package diff" function, which maybe wouldn't be too hard?

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

No branches or pull requests

3 participants