Open phadej opened 6 years ago
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.
https://github.com/haskell/cabal/pull/4383 caused this.
@Ericson2314 is it easy (or possible) to fix without big (including data-type) changes?
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.
And we'd technically need to like phantom type GenericPackageDescription
assert its been checked, too, for a real type-directed solution.
@phadej I think what would be easiest is to do the 2.1
branch, land the refactors, find the fix, and then backport it.
@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.
Also note that #4383 landed only 8 days ago. IMO too late for release.
@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.
As of https://github.com/haskell/cabal/pull/5148 this is just a problem on master.
This is going to be a problem for 2.4.
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...).
How close is 2.4?
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.
Ah OK. Then hacks are in order.
The only thing I'd say is https://github.com/haskell/cabal/pull/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.
I'm remilestoning this from 3.0.1.0 to 3.4. We are again "right before release".
As an additional datapoint, this still occurs on
cabal-install version 3.2.0.0
compiled using version 3.2.0.0 of the Cabal library
I have this exact problem:
[nix-shell:~/code/redacted]$ cabal --version
cabal-install version 3.2.0.0
compiled using version 3.2.0.0 of the Cabal library
[nix-shell:~/code/redacted]$ ghc --version
The Glorious Glasgow Haskell Compilation System, version 8.8.3
[nix-shell:~/code/redacted]$ cat redacted.cabal
cabal-version: >=1.10
name: redacted
version: 0.1.0.0
synopsis:
description:
bug-reports:
license: MIT
license-file: LICENSE
author: evertedsphere
maintainer: evertedsphere@gmail.com
copyright:
category:
build-type: Simple
extra-source-files: CHANGELOG.md
library
exposed-modules: Lib
other-modules:
other-extensions:
build-depends: base >=4.13 && <4.14
hs-source-dirs: src
default-language: Haskell2010
executable redacted-demo
main-is: Main.hs
other-modules:
other-extensions:
build-depends:
base >=4.13 && <4.14,
redacted
hs-source-dirs: exe
default-language: Haskell2010
test-suite redacted-test
default-language: Haskell2010
type: exitcode-stdio-1.0
hs-source-dirs: test
main-is: Test.hs
build-depends: base >=4.13 && <4.14
FWIW, I've just upgraded Cabal from 3.0 to 3.2 and GHC from 8.6 to 8.10, and I'm hitting this where I wasn't before. I don't think I've made any other changes.
After updating cabal-install
from 3.0.0.0 to 3.2.0.0, I'm now getting this warning. I have not changed my Cabal file, nor have I changed compilers (I'm using GHC 8.6.5).
Just commenting to cross reference #4203, which references #4169, a duplicate of this issue I just closed.
Still a problem (cabal-3.2.0.0 here).
Just wanted to chime in: Same here with the sbv package. https://hackage.haskell.org/package/sbv
just discovered this myself. Will this be fixed in cabal 3.4?
Will this be fixed in cabal 3.4?
No.
Is there any way to suppress it ?
On Thu, Nov 12, 2020 at 3:54 AM Oleg Grenrus notifications@github.com wrote:
Will this be fixed in cabal 3.4?
No.
— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/haskell/cabal/issues/5119#issuecomment-725971109, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAABBQQ7SBRFPQMGY5X2XJDSPOWEJANCNFSM4EPLUDEQ .
I'll be happy to review a patch fixing the underlying issue.
Where does the bug live ? What’s acceptance criteria for quick vs systematic fix or what do they entail?
If I knew, I'd probably fixed it already :)
Hrmm: @typedrat @fgaz @Hvr any ideas on where to start?
I am not qualified enough to make a patch but... Offending commit is https://github.com/haskell/cabal/commit/6154796aa634f236750b9c89c0cc38715ffd6780
When flattenTaggedTargets
overwrites targetBuildDepends
in redoDB
func it changes dependency structure like:
Dependency (PackageName "base") AnyVersion
=>
Dependency ( PackageName "base", IntersectVersionRanges AnyVersion (ThisVersion (mkVersion [ 4 , 10 , 1 , 0 ])))
And it breaks checks later on.
If I remove redoBD
warnings go away and my test example still works. But I assume original author introduced it for a reason. Any idea how to approach fixing it?
+1
@emilypi cc :)
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?
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
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?
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.
@gbaz I think we discovered that there was no case in which that error was ever needed or even accurate.
ok, plonk
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.
@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.
Just to bump with the relevant commentary: https://github.com/haskell/cabal/pull/7470#issuecomment-877596613
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).
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.
@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.
@emilypi You asked a good question: nobody looked into. (I don't know either).
There aren't such additional version restriction:
To help bisecting:
previous
cabal-install-head
(from around 2017-01-15) didn't had this issue, IIRC.