Improve error message when multiple .cabal files are in the same directory

[fendor]

closes #7395

---

Please 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.
• The documentation has been updated, if necessary.

[fendor]

Going to improve the error message as soon as I understood what the KnownPackages actually represent.

[jneira]

@fendor i guess your knowledge about KnownPackages has improved 😉
This a nice improvement in an area that needs to be imrpoved a lot

[jneira]

any chance to make definitive the pr?

[fendor]

Unfortunately, changing this requires a bit more refactoring than hoped...

From KnownPackages, we can obtain the .cabal filepath, but cabal resolves somewhat weird:

Error: cabal: Multiple packages have been found:
multiple-cabal-files-0.1.0.0 defined in: "./multiple-cabal-files.cabal"
multiple-cabal-files-0.1.0.0 defined in: "./multiple-cabal-files.cabal"

Although the latter is defined in another cabal file named multiple-cabal-files2.cabal. Makes you wonder why cabal fails at all.

[fendor]

I have a fix now, but it feels slightly brittle. We need a better strategy to track where a package originates from...

[jneira]

We need a better strategy to track where a package originates from...

Yeah, i think we are not tracking enough information in types across the different build phases. The use of monoids in the config f.e. causes a form of "monoid blindness` and we are losing the origin of each value. If phases, along with its inputs and outputs would be encoded in types keeping their sources until the end i think we could improve error messages and make some bugs harder to arise.

There would be a Config Global a Config Cli, a Config ProjectFile and the final monoid folding should be done on the fly, without losing the info of where each value was defined.

[fendor]

Just typing these are probably not enough. It still seems like monkey patching, even if we track the provenancen of FieldDescr entries for this specific issue.

[gbaz]

I think this suffice for now as a clear improvement, and if you resolve the conflicts I think we should seek more reviews and merge.

[fendor]

I feel like someone had a stroke while writing this comment.

[fendor]

I currently do not understand why the test has changed, while it might be benign, I would like to understand why we resolve to a different target now.

[fendor]

Suggested change

	-- Test that we can resolve a module name ambiguity when reexporting
	-- by explicitly specifying what package we want.

[fendor]

I have no idea why the test fails... must be some tricky interaction...

[Mikolaj]

The error is here, right?

 tests/IntegrationTests2.hs:205:
        expected: [TargetPackage TargetExplicitNamed [PackageIdentifier {pkgName = PackageName "p", pkgVersion = mkVersion [0,1]}] Nothing,TargetPackage TargetExplicitNamed [PackageIdentifier {pkgName = PackageName "p", pkgVersion = mkVersion [0,1]}] Nothing,TargetPackage TargetExplicitNamed [PackageIdentifier {pkgName = PackageName "p", pkgVersion = mkVersion [0,1]}] Nothing,TargetPackage TargetExplicitNamed [PackageIdentifier {pkgName = PackageName "p", pkgVersion = mkVersion [0,1]}] Nothing,TargetPackage TargetExplicitNamed [PackageIdentifier {pkgName = PackageName "q", pkgVersion = mkVersion [0,1]}] Nothing,TargetPackage TargetExplicitNamed [PackageIdentifier {pkgName = PackageName "q", pkgVersion = mkVersion [0,1]}] Nothing,TargetPackage TargetExplicitNamed [PackageIdentifier {pkgName = PackageName "q", pkgVersion = mkVersion [0,1]}] Nothing,TargetPackage TargetExplicitNamed [PackageIdentifier {pkgName = PackageName "q", pkgVersion = mkVersion [0,1]}] Nothing,TargetPackage TargetExplicitNamed [PackageIdentifier {pkgName = PackageName "q", pkgVersion = mkVersion [0,1]}] Nothing]
         but got: [TargetPackage TargetExplicitNamed [PackageIdentifier {pkgName = PackageName "p", pkgVersion = mkVersion [0,1]}] Nothing,TargetPackage TargetExplicitNamed [PackageIdentifier {pkgName = PackageName "p", pkgVersion = mkVersion [0,1]}] Nothing,TargetPackage TargetExplicitNamed [PackageIdentifier {pkgName = PackageName "p", pkgVersion = mkVersion [0,1]}] Nothing,TargetComponent (PackageIdentifier {pkgName = PackageName "p", pkgVersion = mkVersion [0,1]}) (CLibName LMainLibName) (FileTarget "P"),TargetPackage TargetExplicitNamed [PackageIdentifier {pkgName = PackageName "q", pkgVersion = mkVersion [0,1]}] Nothing,TargetPackage TargetExplicitNamed [PackageIdentifier {pkgName = PackageName "q", pkgVersion = mkVersion [0,1]}] Nothing,TargetPackage TargetExplicitNamed [PackageIdentifier {pkgName = PackageName "q", pkgVersion = mkVersion [0,1]}] Nothing,TargetPackage TargetExplicitNamed [PackageIdentifier {pkgName = PackageName "q", pkgVersion = mkVersion [0,1]}] Nothing,TargetComponent (PackageIdentifier {pkgName = PackageName "q", pkgVersion = mkVersion [0,1]}) (CLibName LMainLibName) (FileTarget "Q")]

[Mikolaj]

Perhaps the version is expected and should just be committed as the new results of the test? Aren't you changing just these bits of the code?

[fendor]

The error is here, right?

Right

Perhaps the version is expected

As long as I don't understand why the value has changed, it is not expected.
Afaict, nothing I changed in this PR should have affected this... I had one suspicion what might have changed, but it proved to be either insufficient or not the root cause.

should just be committed as the new results of the test

Yeah, that might be the solution, but I at least would feel better if I understood the reason for the changed value.

[fendor]

As it turns out, the failing test-case is genuine.

[jneira]

make this a record would suppose lot of changes? field names would help a lot in understand the two params

[Mikolaj]

How about a changelog?

[fendor]

I am not sure why, but I think like my tests are not picked up by CI

[Mikolaj]

borked rebase?

[jneira]

I am not sure why, but I think like my tests are not picked up by CI

cause they are succesful? they are not printed out in logs, only failed, known broken and skipped ones:

=== cabal-install: cabal-testsuite ============================= 09:28:38 ===
>>> **/cabal-tests --builddir=**/cabal-testsuite-3 --with-cabal=**/cabal -j2 --hide-successes 
threads: 2
tests to run: 306
PackageTests/NewSdist/ManyDataFiles/many-data-files.test.hs                                                       SKIP no open file limit (0.05s)
PackageTests/NewBuild/T4375/cabal.test.hs                                                                         SKIP needs ghc<8.1 (0.02s)
PackageTests/CustomPlain/cabal.test.hs                                                                            SKIP needs ghc<8.2 (0.02s)
PackageTests/CustomDep/cabal.test.hs                                                                              SKIP needs ghc<8.2 (0.02s)
PackageTests/InternalVersions/BuildToolDependsExtra/setup.test.hs                                                 OK (known failure) (0.95s)
PackageTests/InternalVersions/BuildToolsExtra/setup.test.hs                                                       OK (known failure) (0.96s)
PackageTests/InternalVersions/BuildDependsExtra/setup.test.hs                                                     OK (known failure) (0.94s)
SKIPPED 4 tests
OK

https://github.com/haskell/cabal/runs/5316369875?check_suite_focus=true

make them fail cherry picking tests in a branch based on master to check ;-)

[fendor]

But the test doesn't succeed locally any more T_T I am so confused

[jneira]

But the test doesn't succeed locally any more T_T I am so confused

Not sure if it can help but last master workflow run has 305 tests and no 306:

=== cabal-install: cabal-testsuite ============================= 08:47:50 ===
>>> **/cabal-tests --builddir=**/cabal-testsuite-3 --with-cabal=**/cabal -j2 --hide-successes 
threads: 2
tests to run: 305
PackageTests/NewSdist/ManyDataFiles/many-data-files.test.hs                                                       SKIP no open file limit (0.04s)
PackageTests/NewBuild/T4375/cabal.test.hs                                                                         SKIP needs ghc<8.1 (0.02s)
PackageTests/CustomPlain/cabal.test.hs                                                                            SKIP needs ghc<8.2 (0.03s)
PackageTests/CustomDep/cabal.test.hs                                                                              SKIP needs ghc<8.2 (0.02s)
PackageTests/InternalVersions/BuildToolDependsExtra/setup.test.hs                                                 OK (known failure) (0.92s)
PackageTests/InternalVersions/BuildToolsExtra/setup.test.hs                                                       OK (known failure) (0.95s)
PackageTests/InternalVersions/BuildDependsExtra/setup.test.hs                                                     OK (known failure) (0.91s)
SKIPPED 4 tests
OK

https://github.com/haskell/cabal/runs/5315945053?check_suite_focus=true

[Mikolaj]

borked local rebase?

[fendor]

I rebased again to make sure, but I think everything's "in order" locally.

[ulysses4ever]

This looked very close to completion. @fendor how do you feel about bringing it over the line?

[fendor]

Unfortunately, it is not that close, I am failing to write a proper integration test that proves that this PR actually achieves what it sets out to do.