-
Notifications
You must be signed in to change notification settings - Fork 710
Last part of addressing #415 (fwd-ported) #5183
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
Conversation
GHC 8.4.1-rc1 (GHC 8.4.0.20180224) unfortunately still shipped with a devel snapshot of Cabal-2.1.0.0; but GHC 8.4.1 final will ship w/ lib:Cabal-2.2.0.0 (cherry picked from commit 5066672)
As we can't predict the future, we also place a global upper bound on the lib:Cabal version we know how to interact with: The upper bound is computed by incrementing the current major version twice in order to allow for the current version, as well as the next adjacent major version (one of which will not be released, as only "even major" versions of Cabal are released to Hackage or bundled with proper GHC releases). For instance, if the current version of cabal-install is an odd development version, e.g. Cabal-2.1.0.0, then we impose an upper bound `setup.Cabal < 2.3`; if `cabal-install` is on a stable/release even version, e.g. Cabal-2.2.1.0, the upper bound is `setup.Cabal < 2.4`. This gives us enough flexibility when dealing with development snapshots of Cabal and cabal-install. This addresses haskell#415 (cherry picked from commit e66106c)
(cherry picked from commit 86de9cc)
The `setup-custom2` tests implicit `setup-depends` which implies a `Cabal < 2` upper bound, which is incompatible with GHC 8.2 or later (cherry picked from commit 8947db4)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a few comments on the update to the regression test, though I don't mind making the changes myself after the PR is merged, since I wrote the test.
-- between 'Cabal' and the new 'time'. | ||
r <- fails $ cabal' "new-build" ["time", "--constraint=any.time==99999", "--dry-run"] | ||
assertOutputContains "cyclic dependencies; conflict set: time:setup.Cabal, time:setup.time" r | ||
r <- fails $ cabal' "new-build" ["time", "--constraint=any.time==99999", "--constraint=setup.Cabal installed", "--dry-run"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does this command need the setup.Cabal installed
constraint now? It seems like cabal can only choose the installed Cabal library now that it enforces a max setup Cabal version. I thought that the two new-build commands should only differ by the constraint scope, to show that the constraint on time
succeeds, but the constraint on any.time
fails.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was mostly paranoid and wanted the resulting output to be as deterministic as possible but I realize this wasn't probably the most principled way to retain the spirit of this test...
...and I gladly take your offer on changing the tests after the PR is merged, as you clearly have a better understanding on how to write this regression test in an effective way. :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made a PR: #5186
-- between 'Cabal' and the new 'time': | ||
-- assertOutputContains "cyclic dependencies; conflict set: time:setup.Cabal, time:setup.time" r | ||
-- However, this doesn't work anymore, so instead we more directly look for: | ||
assertOutputContains "time:setup.time~>time-99999 (conflict: time:setup.Cabal" r |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I understand this assertion. Shouldn't it check the line that rejects the installed time due to the command line flag, now that the cycle isn't the cause of the failure?
@@ -4,7 +4,7 @@ cabal-version: >=1.8 | |||
build-type: Custom | |||
|
|||
custom-setup | |||
setup-depends: base, Cabal == 99999 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that Cabal-99999 can be deleted now that it doesn't fit the implicit setup constraints.
…askell#415. This commit addresses the comments in PR haskell#5183.
…askell#415. This commit addresses the comments in PR haskell#5183.
…askell#415. This commit addresses the comments in PR haskell#5183. (cherry picked from commit 2b197f8)
This is a forward-port of #5180 / 7ce486e