Avoid using duplicate global config not adding it in the dummy project context

[jneira]

• Hopefully fixes Using cabal install in a project or install/repl without a project, duplicates global config and rebuilds libs twice in the store #6906 following @phadej and @gbaz suggestions

• Afaics all establishDummyProjectBaseContext uses in the codebase (CmdInstall, CmdRepl and CmdRun) are guarded by ProjectOrchestation.withProjectOrGlobalConfig and it is already adding the global config

  • In the runInstall with a project it is included too

• ProjectOrchestation.establishDummyProjectBaseContext

  • Changed the argument name from cliConfig to projectConfig to make clear it includes the config from more sources than the cli.
  • Added an argument comment to make clear client code must include the global config if it is needed

• Patches conform to the coding conventions.

• Any changes that could be relevant to users have been recorded in the changelog.

• Rebase this onto Log package hash inputs file and contents / regression test for #6906 #7759, unmarking knowBroken in the regression tests added there
  Tests:

• From a functional pov afaik the unique observable change is the bad input in hash.txt.. Not sure if it is already being examined in other tests

• Would love to have some help about how and where to add unit tests

[jneira]

The pr still misses the repl and run actions in a project context cases

[jneira]

The pr still misses the repl and run actions in a project context cases

It is already covered indeed cause establishDummyProjectBaseContext is called inside the without project code branch

[jneira]

Trying to discover tests, i did not find:

• any occurrence of withProjectOrGlobalConfig, establishDummyProjectBaseContext, replAction, runAction, installAction nor the correlated *Command functions outside prod code so i assume there is no unit tests for any of them
• any occurrence of hash.txt outside prod code, so i assume there is no integration tests checking it
• any tests checking the contents of the store (without an exhaustive search tbh)

So one path of action i can think of could be add the output of hash.txt in the cabal output with v3 (in another pr?) and check for the correct extra-*-dirs value setting it in the global config

[Mikolaj]

Yes, the test suite has surprising gaps, in particular, not testing some of its own machinery. ;)

[mouse07410]

I'm confused by the title of this PR.

I thought the "final" project context should be:

• global context taken from ~/.cabal/config, then
• local context from {<project>.cabal plus cabal.project plus cabal.project.local} that overrides, modifies, and complements the global context.

Is that what your PR enforces? If not, I'd love to see an explanation.

[jneira]

well the problem is the global config was being added twice, hence the duplication of some config options
so the pr removes the inclusion of global config in one call site (establishDummyProjectBaseContext), after checking all client code is already including such config before calling it

will try to rephrase the title to make it clearer

[jneira]

Tests:

* From a functional pov afaik the unique observable change is the bad input in hash.txt.. Not sure if it is already being examined in other tests

* Would love to have some help about how and where to add unit tests

Regression tests added in #7759, will rebase this after being merged

[jneira]

I have to thanks @vituscze their analysis of the bug here, which has made lot easier this fix

[jneira]

Demoting to draft until #7759 is merged in master

[jneira]

@Mergifyio rebase master

[mergify]

rebase master

✅ Branch has been successfully rebased

[jneira]

test has failed successfully ☺️

PackageTests/Regression/T6906/cabal.test.hs FAIL (unexpected success) (2.36s)

[Mikolaj]

Well spotted. LGTM.

[jneira]

ci has failed quite misteriously:

Actual output differs from expected:

stderr:
--- /Users/runner/work/cabal/cabal/cabal-testsuite/PackageTests/Regression/T6906/cabal.dist/cabal.out.normalized	2021-10-18 17:33:18.000000000 +0000
+++ /Users/runner/work/cabal/cabal/cabal-testsuite/PackageTests/Regression/T6906/cabal.dist/cabal.comp.out.normalized	2021-10-18 17:33:18.000000000 +0000
@@ -0,0 +1 @@

🤔

[Mikolaj]

cabal-testsuite/src/Test/Cabal/Monad.hs--- | The file where the expected output of the test lives
cabal-testsuite/src/Test/Cabal/Monad.hs-testExpectFile :: TestEnv -> FilePath
cabal-testsuite/src/Test/Cabal/Monad.hs-testExpectFile env = testSourceDir env </> testName env <.> "out"
cabal-testsuite/src/Test/Cabal/Monad.hs-
cabal-testsuite/src/Test/Cabal/Monad.hs--- | Where we store the actual output
cabal-testsuite/src/Test/Cabal/Monad.hs-testActualFile :: TestEnv -> FilePath
cabal-testsuite/src/Test/Cabal/Monad.hs:testActualFile env = testWorkDir env </> testName env <.> "comp.out"

Not sure what that means.

[Mikolaj]

Perhaps expectBroken changes output? That would be sad.

[jneira]

Tests has worked locally with b1f7bbe, adding recordMode DoNotRecord

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

Lines 684 to 687 in 980a532

	recordMode :: RecordMode -> TestM a -> TestM a
	recordMode mode = withReaderT (\env -> env {
	testRecordUserMode = Just mode
	})

I am still gazing at the code trying to understand why

[mouse07410]

Great! So, this one is merged.

Could you tell me when can I expect to see it in a released cabal-install?