Spurious warning: The package has an extraneous version range for a dependency on an internal library

[phadej]

% cabal new-build cabal-plan:exe:cabal-plan
Resolving dependencies...
Build profile: -w ghc-8.2.2 -O1
In order, the following will be built (use -v for more details):
 - cabal-plan-0.3.0.0 (lib) (first run)
 - cabal-plan-0.3.0.0 (lib:topograph) (first run)
 - cabal-plan-0.3.0.0 (exe:cabal-plan) (first run)
Configuring library for cabal-plan-0.3.0.0..
Configuring library 'topograph' for cabal-plan-0.3.0.0..
Preprocessing library for cabal-plan-0.3.0.0..
Building library for cabal-plan-0.3.0.0..
Preprocessing library 'topograph' for cabal-plan-0.3.0.0..
Building library 'topograph' for cabal-plan-0.3.0.0..
[1 of 1] Compiling Cabal.Plan       ( src/Cabal/Plan.hs, /home/ogre/Documents/shared-haskell/cabal-plan/dist-newstyle/build/x86_64-linux/ghc-8.2.2/cabal-plan-0.3.0.0/build/Cabal/Plan.o )
[1 of 1] Compiling Topograph        ( src-topograph/Topograph.hs, /home/ogre/Documents/shared-haskell/cabal-plan/dist-newstyle/build/x86_64-linux/ghc-8.2.2/cabal-plan-0.3.0.0/l/topograph/build/topograph/Topograph.o )
Configuring executable 'cabal-plan' for cabal-plan-0.3.0.0..
Warning: The package has an extraneous version range for a dependency on an
internal library: cabal-plan -any && ==0.3.0.0 && ==0.3.0.0. This version
range includes the current package but isn't needed as the current package's
library will always be used.

There aren't such additional version restriction:

executable cabal-plan
  default-language:    Haskell2010
  other-extensions:    RecordWildCards

  main-is: src-exe/cabal-plan.hs
  other-modules: Paths_cabal_plan

  if flag(exe)
    -- dependencies w/ inherited version ranges via 'cabal-plan' library
    build-depends: cabal-plan
                 , topograph
....

To help bisecting:

ii  cabal-install-head                              2.1+git20180202.0.2773999-5~16.04             amd64        Command-line interface for Cabal and Hackage

previous cabal-install-head (from around 2017-01-15) didn't had this issue, IIRC.

[Ericson2314]

Yeah this me. My refactor PRs should fix this, but if there are too much for 2.1 should we do something else? You can bisect the test suit to see when.

[Ericson2314]

#4383 caused this.

[phadej]

@Ericson2314 is it easy (or possible) to fix without big (including data-type) changes?

[Ericson2314]

Actually, there might be. The type changes more help make sure its done correctly than fix the problem in itself. Basically, i need to make sure that checking always comes after whatever elaborates the package-local build tool depends.

[Ericson2314]

And we'd technically need to like phantom type GenericPackageDescription assert its been checked, too, for a real type-directed solution.

[Ericson2314]

@phadej I think what would be easiest is to do the 2.1 branch, land the refactors, find the fix, and then backport it.

[phadej]

@Ericson2314 than I'd opt to revert #4383 in 2.2 branch, this warning is so annoying (and plain wrong), we cannot have release with this either.

EDIT because there's hardly time to review refactoring, at least I don't have. I'm very very sorry.

[phadej]

Also note that #4383 landed only 8 days ago. IMO too late for release.

[Ericson2314]

@phadej No worries, I'm fine with reverting on 2.2. I meant backporting the fix without refactors, to be clear, but agreed that last PR is not that much less invasive than the others.

[Ericson2314]

As of #5148 this is just a problem on master.

[quasicomputational]

This is going to be a problem for 2.4.

[quasicomputational]

How about just knocking the warning down to an info, as a temporary hack, with the intention of bumping it back up once we're no longer generating the spurious warnings with a proper fix? #4383 seems good and we'd want to keep it in; also, I've got no idea how well a revert would apply six months later (and especially not so close to release...).

[Ericson2314]

How close is 2.4?

[quasicomputational]

AIUI, any day now. Ben Gamari said that the 8.6 beta released a few days ago is likely to be the final GHC pre-release, and making the official GHC 8.6.1 release blocks on Cabal 2.4 being cut.

[Ericson2314]

Ah OK. Then hacks are in order.

[Ericson2314]

The only thing I'd say is #4265 might fix it. Furthermore, it used to work for all but 7.4, and now that is removed, so I can try rebasing it again. But it's also a bigger change before the release.

[Mikolaj]

Hi everybody! Would you like to work on this issue? Last moment to snatch it before it gets assigned to an AI robotic assembly for automatic fixing. Seriously, @fgaz just switched the milestone, which means it will be fixed RSN. Please contribute --- there is even a hint in a recent comment pinpointing the offending line (no refunds if it's more than one line).

@Ericson2314: any extra hint from you?

[ulysses4ever]

The finding by @quetz above is nice but simply removing redoBD won't suffice as it breaks tests (unsurprisingly). E.g. for installLatest (cabal run cabal-install:unit-tests -- -p installLatest) I get:

        Exception: internal error: could not construct a valid install plan.
        The proposed (invalid) plan contained the following problems:
        Package B-2.0.0 has an invalid configuration, in particular:
          the package configuration specifies A-1.0.0 but (with the given flag assignment) the package does not actually depend on any version of that package.
        
        Proposed plan:
        PreExisting A-1.0.0 (A-1)
        Configured B-2.0.0 lib

[gbaz]

Now that we've just commented out the offending check for now, what should we do with this issue? Keep it around as a reminder to try to re-enable it fixed? Ignore it?

[gbaz]

Honestly I don't think this check is important enough to do further work on -- its just a "cleanliness" check that can be disregarded, so I vote close.

[emilypi]

@gbaz I think we discovered that there was no case in which that error was ever needed or even accurate.

[gbaz]

ok, plonk

[phadej]

It is accurate

If user writes:

name: my-package
version: 0.1

library
  ...
  
test-suite my-tests
  ...
  build-depends: my-package ==0.1

And this is silly. Right now Hackage rejects such packages. With the fix in, it will accept them.

This is not the end of the world, but it is a regression. There shouldn't be such extraneous version bounds, the check in Check was right, it's internals of Cabal (iirc solver?) which add extra constraints to make solution correct. Why so, it's unclear, and doesn't seem right way to go.

[emilypi]

@phadej in that case, wouldn't the solver always spit out either a build plan with that exact version, or no plans whatsoever (because differing versions on internal libraries is nonsensical)? In that case, the warning is either an error (no build plan could be found), or it warns you about valid syntax being valid (which is not a warning scenario). Unless we plan on introducing internal-build-depends or adding this to cabal check, the warning seems more problematic than leaving it alone.

[emilypi]

Just to bump with the relevant commentary: #7470 (comment)

[phadej]

adding this to cabal check

The current "fix" was to remove that check. I think it's not a "fix", but rather a (hopefully) temporary workaround. I want to see that check reintroduced. This is a principle: there is a condition where we want to issue a warning, than we should be able to.

Currently we cannot because (for whatever reason) there are false positives. cabal-install does something it should not do (edits a GPD).

[phadej]

I want to see cabal-install and Cabal(-syntax) being treated as separate projects (compare base and GHC, they just happen to be in the same repository). Adding that check back does not break anything in Cabal. The bug is in cabal-install, so the fix should be there too.

[emilypi]

@phadej so the question for me now reviewing that pull request again, is why the warning was being thrown during general build, when it's clearly defined for check? The problem that I have currently with it is that it's not an optional check. If it were an optional check I don't think anyone would mind and Ed would not have run into this problem.

So the question now is why was it entangled with the general build workflow? It seems to me that a lot of our problems stem from the way that cabal is architectured leads to intense coupling where there need not be any.

And I still don't agree that it's even a valid error considering it's caught by the solver, an architectural problem on our end, and it's valid syntax. Worse than being a bug, it's straight up confusing and poor UX.

[phadej]

@emilypi You asked a good question: nobody looked into. (I don't know either).