Skip to content

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

Closed
wants to merge 2 commits into from
Closed

Conversation

mightybyte
Copy link
Collaborator

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.

@23Skidoo
Copy link
Member

Would be easier to review if you rebased and got rid of spurious merge commits.

@23Skidoo
Copy link
Member

/cc @dcoutts

@mightybyte
Copy link
Collaborator Author

@23Skidoo If it would be easier I could create a new branch, collapse everything and issue a new pull request.

@mightybyte
Copy link
Collaborator Author

Ok, I just pushed a rebase.

@ttuegel
Copy link
Member

ttuegel commented Aug 18, 2015

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 cabal gen-bounds as written in stone.

Let us consider a library foo which depends on bar, and some other packages baz and quux which depend on foo. What may happen when a new version of bar is released if foo has a speculative upper bound on bar?

  1. foo would not build with the new bar.
    Good! The upper bound is working as intended.
  2. foo would build with the new bar.
    Bad! When the dependency solver rules out a configuration that would build, this is sometimes reported as a bug in cabal-install, although it is actually a bug in foo.
    Workaround: Use --allow-newer. Caveat: This is a global flag and sometimes exposes problems in other packages. For example, if baz has required upper bounds, it cannot be built with --allow-newer.
    Solution: Release a new version of foo or edit the package description on Hackage to relax the upper bound or bar. The maintainer may have no feedback that this is necessary, so it could take some time before the problem is discovered. Caveat: If quux also depends on bar, but has a less restrictive upper bound (which the PVP does allow!), then quux will fail to build with the new foo if its upper bound was not correct. This is a bug in quux, and not strictly foo's problem, but this is where Haskell and cabal-install get their reputation for difficult-to-debug build problems.

Let us consider instead what may happen if foo does not have a speculative upper bound on bar:

  1. foo does not build with the new bar.
    Bad! This is a bug in foo.
    Workaround: Use --constraint. The added constraint cannot affect other packages adversely.
    Solution: Edit the package description on Hackage to add a real upper bound. This is likely to happen quickly because the maintainer has immediate, direct feedback that it is necessary. Adding an upper bound or making an existing upper bound more restrictive cannot break existing, working builds.
  2. foo does build with the new bar.
    Good! No intervention necessary.

In both cases, there is a build failure, but here's the difference: If foo has speculative upper bounds, the effect of fixing the build failure may be to break other packages. These breakages may be difficult to detect because they can be far from the source (bar). If the build fails due to a lack of upper bounds, however, the failure is immediate, close to the source, and fixing the failure does not break other packages.

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 foo's dependencies; if foo is an executable, it has none.

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 cabal freeze. For old code without upper bounds, and without a frozen configuration, adding the PVP-recommended upper bounds is likely to produce a working configuration. (I am 100% in favor of having an option to generate restrictive upper bounds for libraries, I just don't think it should be on by default.) If these two approaches are not sufficient, then we need to develop new ones; we should not make code archival easier at the expense of making active maintenance harder!

@bitemyapp
Copy link
Contributor

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 cabal gen-bounds as written in stone.

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.

@gbaz
Copy link
Collaborator

gbaz commented Aug 18, 2015

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.

@ttuegel
Copy link
Member

ttuegel commented Aug 18, 2015

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.

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 cabal gen-bounds to generate their version bounds at first. (Good; that's its purpose.) The new user will say: "These upper bounds were generated by cabal-install for me, what could be wrong with them?" and they'll carry on doing upper bounds that way. We will have trained them to do upper bounds in a way that I think will eventually cause them pain. That's the root of my objection.

@23Skidoo
Copy link
Member

UI bikeshedding: I wonder whether gen-bounds should be a part of cabal check.

@cartazio
Copy link
Contributor

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.

@23Skidoo
Copy link
Member

@ttuegel

Workaround: Use --allow-newer. Caveat: This is a global flag and sometimes exposes problems in other packages. For example, if baz has required upper bounds, it cannot be built with --allow-newer.

#2756 suggests a way to lift this restriction. Hopefully will get around to implementing it soon.

@cartazio
Copy link
Contributor

maybe it'd be worth asking the various active hackage trustees which default they think will help the most folks @hvr @bergmark thoughts?

@gbaz
Copy link
Collaborator

gbaz commented Aug 19, 2015

If people view the contents of the commit they will see it has a very helpful message included, which includes the following:

+         , "Note that version bounds are a statement that you've successfully built and"
+         , "tested your package and expect it to work with any of the specified package"
+         , "versions (PROVIDED that those packages continue to conform with the PVP)."
+         , "Therefore, the version bounds generated here are the most conservative"
+         , "based on the versions that you are currently building with.  If you know"
+         , "your package will work with versions outside the ranges generated here,"
+         , "feel free to widen them."

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.

@hvr
Copy link
Member

hvr commented Aug 19, 2015

@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 .cabal edits).

@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. cabal is already doing this in some places, and so is hackage-server (by rejecting uploads which lack an upper bound on base, references to the PVP, and there are a couple of more things that are frequently suggested), and finally the Hackage trustees were formed to improve the health of Hackage by fixing bounds and more importantly educate package authors about the use of proper bounds.

I consider the prime directive for Hackage trustees to achieve and preserve the following invariant for Hackage meta-data:

  • An invocation of cabal install $PKG shall never result in a compile error.

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.

@ttuegel
Copy link
Member

ttuegel commented Aug 19, 2015

I withdraw my objections.

@23Skidoo I think cabal check still complains if you don't have upper bounds, is that right? In that case, I agree with you that this output should be part of cabal check.

@cartazio
Copy link
Contributor

I support hvr's stance as articulated. I withdraw my opinions to the
contrary.

On Wednesday, August 19, 2015, Thomas Tuegel notifications@github.com
wrote:

I withdraw my objections.

@23Skidoo https://github.com/23Skidoo I think cabal check still
complains if you don't have upper bounds, is that right? In that case, I
agree with you that this output should be part of cabal check.


Reply to this email directly or view it on GitHub
#2774 (comment).

@bergmark
Copy link
Contributor

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.

@bitemyapp
Copy link
Contributor

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.

@Andrew-the-Ubergeek
Copy link

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

If you know your package will work with versions outside the ranges generated here, PLEASE widen them.

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

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.

...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.

@ttuegel
Copy link
Member

ttuegel commented Sep 21, 2015

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 pvp-bounds instead? I think this name is better because it says how the bounds are being generated. If I was coming at this with only a little Haskell experience, I wouldn't necessarily expect gen-bounds to generate bounds that conform to the PVP.

@mightybyte
Copy link
Collaborator Author

@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.

@ttuegel

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?

Yes, that's correct.

Could we call this pvp-bounds instead?

We could. I don't have a strong opinion either way. But I think I have a small preference for the current gen-bounds because I think it fits better with most of the existing commands which are verbs. When I only had a little Haskell experience I didn't even know the PVP existed, so I think gen-bounds would have made more sense to me.

@ttuegel
Copy link
Member

ttuegel commented Sep 21, 2015

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.

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:

  1. the bounds are automatically generated suggestions and the user should use their best judgement, in particular:
  2. some dependencies may need to have their upper bounds tightened (refer to the PVP).

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.

@gbaz
Copy link
Collaborator

gbaz commented Sep 21, 2015

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 gen-pvp-bounds to keep both the "verb" aspect but indicate what the notion of the bounds corresponds to?

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...

@ttuegel
Copy link
Member

ttuegel commented Sep 21, 2015

One bit of comprimise bikeshedding here: could we maybe name the command gen-pvp-bounds to keep both the "verb" aspect but indicate what the notion of the bounds corresponds to?

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..."

@mightybyte
Copy link
Collaborator Author

I thought about gen-pvp-bounds too and I prefer the shorter gen-bounds as well.

@ttuegel I simply observed that most of cabal's commands are verbs (update, install, configure, build, test, etc) and chose gen-bounds following that pattern.

@Andrew-the-Ubergeek
Copy link

Update: my preferred option is now:

If cabal warns everyone (including those with existing upper bounds that are on old versions)

Warning: a newer version (2.41) of widgetfoo exists. 
Please test and widen your upper bound if you can." 

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.

@ttuegel

The upper bound is based on the version(s) that the package has been tested with.

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.

@ezyang
Copy link
Contributor

ezyang commented Dec 23, 2015

Hi guys, is this PR intended to be merged or an RFC?

@23Skidoo
Copy link
Member

Yes, I intend to merge this PR. I mentioned it in my status update.

@BardurArantsson
Copy link
Collaborator

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?

@hvr
Copy link
Member

hvr commented Feb 6, 2016

@BardurArantsson what do you mean concretely by providing this information out of band instead? (also, are you aware that cabal init sets a precedent here?)

@BardurArantsson
Copy link
Collaborator

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. :)

@hvr
Copy link
Member

hvr commented Feb 6, 2016

@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... :-)

@23Skidoo
Copy link
Member

#2756 suggests a way to lift this restriction. Hopefully will get around to implementing it soon.

TWIMC: this is now implemented (#3171).

@23Skidoo 23Skidoo added this to the cabal-install 1.24.1 milestone Feb 20, 2016
@23Skidoo
Copy link
Member

Since this PR only touches cabal-install code, I'm postponing it for the 1.24.1 release.

@23Skidoo 23Skidoo modified the milestones: cabal-install 1.24.1, cabal-install 1.24 Feb 23, 2016
@23Skidoo 23Skidoo modified the milestones: cabal-install 1.24, cabal-install 1.24.1 Mar 12, 2016
@23Skidoo
Copy link
Member

Closing in favour of #3223.

@23Skidoo 23Skidoo closed this Mar 12, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.