Switch from stack to cabal-install for building server code
#3558
Conversation
|
Deploy preview for hasura-docs ready! Built with commit a52c513 |
|
Review app for commit e7cd488 deployed to Heroku: https://hge-ci-pull-3558.herokuapp.com |
The idea here is to start the process of moving away from stack (potentially), but making this move official if we decie to, will require more coordination (moving CI, dev.sh and docs all at once). I've added a "fast" variation of cabal.project.local mostly as a demonstration. I haven't found a "prof" variant that really works for me. All this is pretty open to change, but it would be nice to have some local cabal.project files that encode good/useful stuff. The freeze file should be version-exact to our stackage LTS snapshot.
jberryman
force-pushed
the
initial-cabal-install-support-issue-3280
branch
from
e7cd488 to
46169d5
Compare
|
Review app for commit 46169d5 deployed to Heroku: https://hge-ci-pull-3558.herokuapp.com |
|
@lexi-lambda (bump) can we merge or close this? |
|
Review app for commit 9604183 deployed to Heroku: https://hge-ci-pull-3558.herokuapp.com |
|
@jberryman I’d like to merge the |
|
Cool, just didn't want this to get lost. |
…tall-support-issue-3280
|
Review app for commit 3be0134 deployed to Heroku: https://hge-ci-pull-3558.herokuapp.com |
|
Review app for commit b338102 deployed to Heroku: https://hge-ci-pull-3558.herokuapp.com |
|
Review app for commit 7045e91 deployed to Heroku: https://hge-ci-pull-3558.herokuapp.com |
|
Review app for commit 27ca78b deployed to Heroku: https://hge-ci-pull-3558.herokuapp.com |
|
Review app for commit e9f2094 deployed to Heroku: https://hge-ci-pull-3558.herokuapp.com |
|
Review app for commit a76b847 deployed to Heroku: https://hge-ci-pull-3558.herokuapp.com |
|
What about this file: https://github.com/hasura/graphql-engine/blob/master/server/packaging/Dockerfile From initial glance, seems like we don't need it anymore. |
|
Thanks, good catch. I’ve deleted it. |
|
@jberryman Can you have a look at Alexis's changes? Let's get this merged as soon as possible. |
|
LGTM except for porting |
See discussion here: hasura#3584 (review) It should be possible to overwrite that module so the new threadDelay sticks per the pattern in hasura#3705 blocked on hasura#3558 Rename the Control.Concurrent.Extended.threadDelay to `sleep` since a naive use with a literal argument would be very bad! We catch a bug in 'computeTimeDiff'. Add convenient 'Read' instances to the time unit utility types. Make 'Second' a newtype to support this.
|
@jberryman I’ve pushed an update to
I think it would be more fruitful to submit PRs to |
|
I also want this merged. I am struggling with lts not having few packages. And adding them to extra-deps is causing some cascading failures 😇 |
|
For those curious, I needed http://hackage.haskell.org/package/time-1.9.3 because of https://github.com/hasura/cron/blob/e73cab157f2d47f80fe25b77c32212248884d483/stack.yaml#L5 Now, I forget why I needed that there. |
| any.th-lift ==0.7.11, | ||
| any.th-lift-instances ==0.1.12, | ||
| any.these ==0.7.6, | ||
| any.time ==1.8.0.2, |
There was a problem hiding this comment.
Probably off-topic, say I need time = 1.9.3 but that has other deps , then how will i figure that out? cabal-install will resolve the plan and suggest changes?
There was a problem hiding this comment.
The easiest way is just to delete the cabal.project.freeze file and re-run cabal new-freeze. The main point of the freeze file is to ensure reproducible builds, so other people won’t get spurious build failures because the state of the package index changed and for some reason the new build plan is somehow broken. It’s to avoid things from changing out from underneath you. There’s nothing wrong with just blowing it away and regenerating it, since once you check it in, you’ve verified that the build plan is, in fact, good.
Stackage is really the same way. Technically it’s true that bumping the LTS version only bumps minor versions, so theoretically it’s “safer” if people are versioning their packages really carefully, but the truth is that Stackage does no auditing of packages to ensure that’s actually true; all that matters about an LTS is that all the packages build together. When Stackage was first created, this was a bigger deal than it is now, since a lot of packages didn’t build together, so cabal-install often failed to generate build plans. But the funny thing about Stackage is that its benefits are actually useful to cabal-install users almost as much as to stack users, since the ecosystem-wide CI means packages are kept more compatible, and cabal-install is able to generate good build plans.
In that light, I feel like there are really relatively few benefits to using Stackage directly, in a “tragedy of the commons” sort of way. cabal-install is more flexible, since you aren’t tethered to a particular snapshot, but you still get working build plans without much effort.
There was a problem hiding this comment.
so other people won’t get spurious build failures because the state of the package index changed
Just for the record (please just ignore me if this is already known); if the main worry is the package-index changing resulting in unsound build-plans, "freezing" merely the index-state can be accomplished by --index-state=<timestamp> instead of freezing the actual build-plan.
But of course, doing a full cabal freeze via version pinning (and you'd still want to combine that with index-freezing) gives more guarantees re reproducibility.
There was a problem hiding this comment.
(and you'd still want to combine that with index-freezing)
Why freeze the index as well? In case a hackage "revision" changes the constraints for a dependency version retroactively?
@lexi-lambda I'd still like a little clarity on how we want to manage package version upgrades and new dependencies as a team. I think it might make sense to pin the index-state in cabal.project (similar to a stackage version) if it allows us to do two things separately:
- add new dependencies without upgrading a bunch of others
- upgrade some or all dependencies to newer versions (and bumping index version)
...but I'm not sure that's how that would work.
There was a problem hiding this comment.
Maybe I’m in the minority on this, but I’ve always been of the opinion that the hand-wringing about Hackage revisions is overblown. I understand why a project like Stackage might find them frustrating, but Stackage has pretty special needs. From my point of view, Hackage revisions are an extremely good thing, as they prevent incorrectly-specified constraints from indefinitely polluting the solver.
My guess is that we won’t ever encounter a situation where a revision causes a build that previously passed to fail. Revisions are almost exclusively added for one of two reasons:
-
A new version of a dependency was released, so someone makes a revision to weaken a conservative upper bound once it’s clear the new version is still compatible.
-
Someone discovered that a selected build plan does not actually successfully compile, so they fix the incorrect bound via revision.
We don’t care about (1), since we’ve pinned all the versions in the freeze file anyway. Weakened bounds can’t affect our build plans. And while it’s theoretically possible that we could get bitten by (2), it’s highly unlikely, as if our build passes CI, then our build plan fundamentally must be a good one. If a package maintainer adds a revision that breaks our previously-working build plan, either they screwed up (and we should tell them about it), or they had a really good reason to do so that we probably want to know about!
Given the above, I don’t think pinning the index state is super useful, and it seems more likely to create busywork than anything else. If it ever becomes a problem, I’m more than happy to revisit this decision, but I don’t expect it to be.
I'd still like a little clarity on how we want to manage package version upgrades and new dependencies as a team.
From my point of view, the right workflow here is really simple: delete the freeze file and re-run new-freeze. Ideally, this should always be possible, as if it isn’t, that means the constraints in our .cabal file are too loose! The purpose of the freeze file in my eyes is strictly build reproducibility: if it turns out the constraints in our .cabal file are too loose, we don’t want to find out because CI jobs just start suddenly failing. But if we find out because we deleted the freeze file, that’s great, as there’s a human in the loop at that point to fix the .cabal file.
In other words, I think manually worrying about the freeze file is an antipattern. We should never rely on the constraints in the freeze file to save us, so we should only be concerned with the .cabal file. That said, if you really want to bump just one dependency, you can either:
-
Update the constraint in the freeze file by hand.
-
Delete the constraint for that particular package from the freeze file and run
new-freezeagain.This should work, as I believe
cabal-installwill take the existing freeze file into account when creating the new build plan. When it regenerates the freeze file, it will include all the existing constraints, plus whatever new ones are needed for the updated dependency.
But really, I think that’s overly pessimistic. If we generate a build plan that (a) compiles and (b) passes CI, then we can be pretty confident it’s a good one. And if anything, we want our dependencies to be as up to date as possible, since we want to integrate bugfix patches as much as we can!
Let me know if that makes sense to you or if you disagree with any of the above. I can also put some of these thoughts about the workflow in the CONTRIBUTING.md if you think that would help.
There was a problem hiding this comment.
Hackage revisions are an extremely good thing,
I agree, I'm not really worried about revisions.
That said, if you really want to bump just one dependency, you can either: ...
I have pretty high confidence in the quality of libraries on hackage generally, but if we have a situation where e.g. many dependencies are changing arbitrarily (maybe even being downgraded?) without much oversight on each PR that sounds a little scary for debugging issues (esp performance issues).
I'm okay trying to proceed this way. But I think I'd be more comfortable with guidance like: "try to upgrade dependencies only as needed by editing the freeze file ... upgrades of many dependencies can happen, but should be in a separate commit so it can be easily reverted if necessary". Does that sound reasonable?
There was a problem hiding this comment.
I don’t expect there to be a ton of churn in the freeze file even if people delete it and recreate it, but I’m willing to try and be conservative if possible if you think that’s a good idea.
From my point of view, there is a fundamental tension here: we want to incorporate bugfix patches as much as possible (perhaps there’s a performance bug or security vulnerability), and we just aren’t going to regularly audit all of our dependencies to see which ones need upgrading. For that reason, my preference is to give package authors the benefit of the doubt and to assume that newer versions are better. Sure, we could get bitten by upgrading if that assumption turns out to be wrong, but we could also get bitten if a package fixes an issue and we haven’t upgraded.
Still, I can also understand wanting to avoid profiling noise that comes from changes in dependencies rather than changes in our code. So I’m willing to be a little more deliberate about this, I just don’t want to be more conservative than we have to be.
|
Well, I did say I would merge this today if I didn’t receive any objections, and I’ve received none, so I’m going to merge this as soon as CI passes. @jberryman, feel free to open a followup PR to further improve |
|
Review app https://hge-ci-pull-3558.herokuapp.com is deleted |
See discussion here: hasura#3584 (review) It should be possible to overwrite that module so the new threadDelay sticks per the pattern in hasura#3705 blocked on hasura#3558 Rename the Control.Concurrent.Extended.threadDelay to `sleep` since a naive use with a literal argument would be very bad! We catch a bug in 'computeTimeDiff'. Add convenient 'Read' instances to the time unit utility types. Make 'Second' a newtype to support this.
Meant to give the thumbs up there. Thanks for doing that! I'll have some updates to the script this week. |
See discussion here: hasura#3584 (review) It should be possible to overwrite that module so the new threadDelay sticks per the pattern in hasura#3705 blocked on hasura#3558 Rename the Control.Concurrent.Extended.threadDelay to `sleep` since a naive use with a literal argument would be very bad! We catch a bug in 'computeTimeDiff'. Add convenient 'Read' instances to the time unit utility types. Make 'Second' a newtype to support this.
…3552 We upload a set of accumulating timers and counters to track service time for different types of operations, across several dimensions (e.g. did we hit the plan cache, was a remote involved, etc.) Also... Standardize on DiffTime as a standard duration type, and try to use it consistently. See discussion here: hasura#3584 (review) It should be possible to overwrite that module so the new threadDelay sticks per the pattern in hasura#3705 blocked on hasura#3558 Rename the Control.Concurrent.Extended.threadDelay to `sleep` since a naive use with a literal argument would be very bad! We catch a bug in 'computeTimeDiff'. Add convenient 'Read' instances to the time unit utility types. Make 'Second' a newtype to support this.
…3552 We upload a set of accumulating timers and counters to track service time for different types of operations, across several dimensions (e.g. did we hit the plan cache, was a remote involved, etc.) Also... Standardize on DiffTime as a standard duration type, and try to use it consistently. See discussion here: hasura#3584 (review) It should be possible to overwrite that module so the new threadDelay sticks per the pattern in hasura#3705 blocked on hasura#3558 Rename the Control.Concurrent.Extended.threadDelay to `sleep` since a naive use with a literal argument would be very bad! We catch a bug in 'computeTimeDiff'. Add convenient 'Read' instances to the time unit utility types. Make 'Second' a newtype to support this.
…hasura#3280) (hasura#3558) Co-authored-by: Alexis King <lexi.lambda@gmail.com>
…3552 We upload a set of accumulating timers and counters to track service time for different types of operations, across several dimensions (e.g. did we hit the plan cache, was a remote involved, etc.) Also... Standardize on DiffTime as a standard duration type, and try to use it consistently. See discussion here: hasura#3584 (review) It should be possible to overwrite that module so the new threadDelay sticks per the pattern in hasura#3705 blocked on hasura#3558 Rename the Control.Concurrent.Extended.threadDelay to `sleep` since a naive use with a literal argument would be very bad! We catch a bug in 'computeTimeDiff'. Add convenient 'Read' instances to the time unit utility types. Make 'Second' a newtype to support this.
See #3280
The idea here is to start the process of moving away from stack
(potentially), but making this move official if we decie to, will
require more coordination (moving CI, dev.sh and docs all at once).
I've added a "fast" variation of cabal.project.local mostly as a
demonstration. I haven't found a "prof" variant that really works for
me. All this is pretty open to change, but it would be nice to have some
local cabal.project files that encode good/useful stuff.
The freeze file should be version-exact to our stackage LTS snapshot.