-
Notifications
You must be signed in to change notification settings - Fork 712
Add the gen-bounds command #2774
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
Would be easier to review if you rebased and got rid of spurious merge commits. |
/cc @dcoutts |
@23Skidoo If it would be easier I could create a new branch, collapse everything and issue a new pull request. |
Ok, I just pushed a rebase. |
I would like to recommend that we do not generate speculative upper bounds on dependencies for libraries by default. The PVP shouldn't recommend this either, but my voice carries no weight there. Speculative upper bounds on dependencies for libraries may be harmful and I fear that newcomers may treat the output of Let us consider a library
Let us consider instead what may happen if
In both cases, there is a build failure, but here's the difference: If I am not opposed to speculative upper bounds on executables, even extremely restrictive ones. The biggest problems I see with speculative upper bounds are passed on to I understand that a lack of upper bounds hurts the "archivability" of old code. We have tools designed to address this, in particular we have |
100% agreed. I agree in spirit with PVP but most upper-bounds triggered breakages are false negatives and the build would've worked anyway. Breakages that could've been avoided by stricter bounds are uncommon and can be dealt with on a case by case basis. I add upper-bounds to my libraries, but I'm trying not to break the solver and I'm a human being that gets pinged by Stackage whenever I'm breaking somebody's stuff. In situations where there isn't a commitment to being told when you need to bump upper bounds, I'm not sure it's worth it on balance. I agree with everything @ttuegel said so far. I also do not believe bounds are an appropriate solution to archival. If somebody really wants something to be reproducible in an archive-grade manner then cabal freeze and/or vendoring are the only real solutions. Those options are available to people who want it without making things harder for programmers and learners here and now. |
We've had this debate for years now and it won't be resolved here. Given that we stand by the PVP, upper bounds and all, cabal should work with that imho. I'm open to e.g. a "--no-upper" flag for people that really don't want it. And nothing is being done "by default" -- rather, you have to ask cabal to do this for you. The ability to retroactively modify bounds means that loosening them can be done very easily as well. tldr: I support the feature and I support the pull. People are welcome to try to tell others that they disagree with the usage of this feature, but that debate shouldn't affect the ability to have the feature there for the large number of people that would like it. |
I wasn't very clear about what I had in mind by "default," so let me expand on that a bit (lest anyone think I didn't actually look at the pull request). Once this command is in cabal-install, people will tell new Haskell users to use |
UI bikeshedding: I wonder whether |
I was talking with a few folks, and I'd like to propose a middle ground on the default output for this command. Namely, include the upper bound part of the constraint, but have it commented out in the info. This accommodates both styles of users, and provides "archival" info even if it stays as comments. I do think that version bounds when done thoughtfully are very important, and i think poor bounds are nearly as bad as no bounds. I do think now that we have hackage trustees, and tools like http://matrix.hackage.haskell.org/ (currently down), its cheap to fix bad bounds. part of the reason why theres even any disagreement about whether or not to make upper bounds included in the suggested output stems from the fact that currently the only way to actually debug a bad build plan (that has unsatisfiable constraints) is to do '-v3' on cabal install, and thats not something we expect new users to do. On the flip side, too relaxed constraints materialize as type errors, which are a bit more visible to end users. Both perspectives are valid, and come from diverse tooling/library usage sub ecosystems/packaging styles. point being, I think as a near term choice, including upper bound constraint info in the output, but commented out, strikes a balance that leaves everyone equally unhappy. |
#2756 suggests a way to lift this restriction. Hopefully will get around to implementing it soon. |
If people view the contents of the commit they will see it has a very helpful message included, which includes the following:
I suggest that the feature be taken as-is (since it generates bounds that match PVP policy), and bike-shedding be limited to the explanatory text provided with them. |
@cartazio I'm afraid I have to disagree. It's not cheap to fix upper bounds at all. The matrix-builder often appears down because some packages with complex dependencies combined with lack of proper bounds cause it to get stuck in the install-plan solving process. The Hackage trustee curation doesn't scale when people start leaving off upper bounds. It takes me precious time (which I'd rather spend on improving GHC) to investigate what bounds I need to add and can safely add to a package I haven't authored and maybe never used. Adding bounds in hindsight is a delicate and risky procedure as its effect can spread throughout all of Hackage (I could easily break 90% of Hackage instantly for everyone by just a few strategic @ttuegel You're basically arguing against consistency in tooling and infrastructure. We're working to establish consistency with the PVP (which is rather pointless without speculative upper bounds) in various places. I consider the prime directive for Hackage trustees to achieve and preserve the following invariant for Hackage meta-data:
But we need package authors to help us, as there's a latency involved in detecting the breakage, investigating the proper fix, and implementing the cabal-edit to restore the breakage. If multiple such breakages stack up, they simply prolong the duration of the violation of the invariant and waste everyone's time. Trustees can't be the only ones adding upper bounds everywhere, as that doesn't scale and is error-prone. Speculative upper bounds are the most cost-effective tool we have right now to preserve this invariant for the meta-data. |
I withdraw my objections. @23Skidoo I think |
I support hvr's stance as articulated. I withdraw my opinions to the On Wednesday, August 19, 2015, Thomas Tuegel notifications@github.com
|
So far there has been a few requests for widening bounds, less than package takeover requests. The majority of problems I deal with is without a doubt new releases breaking reverse-dependencies due to missing upper bounds. This often makes all versions of a package unbuildable, so it also takes much more time to fix. As an example note the revisions for fay where i switched to using PVP upper bounds around v0.18.1. A pre-emptive upper bound usually only means that the latest version should be updated. Maintainers have good tools for seeing when to relax upper bounds (packdeps, stackage) but there is currently no easy way to find out that an old version of your package no longer compiles. It pretty much comes down to: missing upper bounds gets dealt with by the trustees and relaxing upper bounds is distributed evenly across the community. |
I withdraw my objections as well. This will need documentation and explanation for the end-users. I couldn't say precisely where is best for that. Could mention as a comment in the generated bounds (along the lines @cartazio mentioned) that it was generated and not necessarily a human-verified upper-bound. |
I feel upper bounds should be accurate. I'd prefer it if this provided two proposed sets of bounds (when they differ) - one with the current very conservative rounding-up of the lower bound, and one with a less conservative upper bound rounding up the current hackage version number. Otherwise long-standing packages that have a lot of backwards compatibility for dependencies may be misled into creating unnecessary bound failures. In a similar vein, I'd prefer the concluding sentence to be
Because I think it's just as important that upper bounds aren't too low than that they aren't too high. Infinity is the high upper bound that will become incorrect over time, but rounding up the lower bound may give you an upper bound that is incorrect straight away. I also agree with
...which would give folks the confidence to try compiling against newer versions, particularly if the auto-generated version is already behind hackage, without believing that there's definitely a problem that the maintainer found. |
Stupid question: If I understand correctly, the PVP upper bound is generated based on the versions of dependencies the package has actually been built with, is that right? Bikeshed: Could we call this |
@Andrew-the-Ubergeek I'd rather not suggest bounds that could possibly be wrong. We could, however, warn the user about dependencies that are behind the most recently released major version.
Yes, that's correct.
We could. I don't have a strong opinion either way. But I think I have a small preference for the current |
It's important to note that both of these suggestions are contrary to the PVP. The upper bound is based on the version(s) that the package has been tested with. The only choice the PVP leaves to the developer is whether to allow minor version upgrades of dependencies. I think the help text should mention:
I don't see the sense in recommending anything else. I disagree with the PVP on ideological grounds, but I have been overruled and this project endorses the PVP. So, we're going to endorse the PVP and nothing else. |
I agree -- the PVP is what it is, and whether or not people even recommend the pvp, it is appropriate that Cabal provide a tool to help those who wish to comply. One bit of comprimise bikeshedding here: could we maybe name the command It would also be good to mention the words "package version policy" in the helptext for the command, and perhaps put a link to it either there or in the explanatory text with the generated bounds... |
Sorry, I didn't mean to open the bikeshed floodgates. :) I would rather have the shorter name than have a PVP reference in the name. First time users are most likely to read the help text first, and nobody is going to find the new command without documentation and examples. I'm confused about the "verb" aspect, though. Unless I'm mistaken, this doesn't modify the package description, it just displays the bounds calculated by freezing the package. I've been thinking of this as a "pure" function, rather than an action; a "noun" as in "The pvp-bounds of your package are..." |
I thought about @ttuegel I simply observed that most of cabal's commands are verbs (update, install, configure, build, test, etc) and chose |
Update: my preferred option is now: If cabal warns everyone (including those with existing upper bounds that are on old versions)
That would go a long way towards encouraging maintainers to keep their bounds accurate, and as this becomes more common, you get more success where possible and early failure where not. Wins all round.
I wasn't intending that package authors pick randomly, only that they're alerted to the fact there are newer versions (which they may well have tested against without doing anything about their upper bound), so they don't accidentally just have the lower bound become the major version upper bound when that's incorrect. |
Hi guys, is this PR intended to be merged or an RFC? |
Yes, I intend to merge this PR. I mentioned it in my status update. |
Though I certainly cannot and shouldn't decide anything on this... I must admit that I'm slightly uncomfortable with a tool providing this level of policy-over-mechanism. Is there some sort of insurmountable problem providing this out-of-band? (I must admit I didn't read the whole thread, just skimmed a few comments.) Would it not be better to "export" the information needed to provide it OoB? |
@BardurArantsson what do you mean concretely by providing this information out of band instead? (also, are you aware that |
Oh, I think someone (on the Haskell subreddit?) actually posted a thing for showing (in a machine-friendly) way everything that Cabal/cabal-install knows about a package (including versions and such) in a machine-friendly way. I was thinking of that kind of thing being helpful to foster innovation outside of having to actually extend Cabal/cabal-install. Also... doesn't this already exist as a package on Hackage? (For the life of me, I cannot recall the name of the package, but I could have sworn there was an announcement of such a thing semi-recently.) Anyway, this really isn't an objection per se... just a "meh, doesn't feel quite right... anyone feel the same way?" type of deal. It's not like it's a big distaster if this gets merged or anything. :) |
@BardurArantsson Well, the big benefit of having features included in cabal proper is discoverability, expresses some level of endorsement, and reduces the threshold for the feature actually being used. You not even remembering how that other package is called seems like a case in point to me... :-) |
Since this PR only touches |
Closing in favour of #3223. |
This command automates the process of figuring out correct upper bounds on dependencies, simultaneously educating users about how upper bounds to be set and making it easy for people to bring packages without bounds into full PVP compliance.