Improve the "pub lish" warnings about dependency constraints.

Improve the "pub lish" warnings about dependency constraints.

This adds a warning for a constraint that only allows a single version
and warnings for constraints with no upper bound or no lower bound. It
also improves the upper bound on suggested pre-1.0 constraints to
follow the current Dart team practice and fixes a bug in
[VersionRange.isAny].

R=rnystrom@google.com
BUG=

Committed: https://code.google.com/p/dart/source/detail?r=32867

[Bob Nystrom, 2014-02-20 01:22:49]

https://codereview.chromium.org/173113004/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/validator/dependency.dart (right):

https://codereview.chromium.org/173113004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/validator/dependency.dart:29:
_warnAboutSingleVersionConstraint(dependency);
I think a single version constraint is potentially legitimate, at least for
packages <1.0.0. Since any version can have breaking changes, it may be that you
only support a single version.

I worry that with this patch and our previous validation what we're really doing
is just training users to ignore the warnings.

https://codereview.chromium.org/173113004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/validator/dependency.dart:32:
_warnAboutNoConstraintLowerBound(dependency);
If you know your package works with the first version of a package, it doesn't
seem unreasonable to omit this. Sure, the package author could later publish a
lower version, but that's very unlikely in practice.

https://codereview.chromium.org/173113004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/validator/dependency.dart:90: "future\n"
I like this, though the bold is a bit much.

https://codereview.chromium.org/173113004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/validator/dependency.dart:105: 'package along with
other packages.');
"other package that also depend on it."

https://codereview.chromium.org/173113004/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/version.dart (right):

https://codereview.chromium.org/173113004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/version.dart:452: bool get isAny => min == null &&
max == null;
The previous looks utterly wrong, which implies it isn't tested. Can you add
some tests for this?

[nweiz, 2014-02-20 01:48:24]

https://codereview.chromium.org/173113004/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/validator/dependency.dart (right):

https://codereview.chromium.org/173113004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/validator/dependency.dart:29:
_warnAboutSingleVersionConstraint(dependency);
On 2014/02/20 01:22:49, Bob Nystrom wrote:
> I think a single version constraint is potentially legitimate, at least for
> packages <1.0.0. Since any version can have breaking changes, it may be that
you
> only support a single version.

That's a harmful thing to declare, though. People who care about semantic
versions tend to use them even for pre-1.0 packages, so it's unlikely that an
"=" constraint will protect users from breakage in practice. On the other hand,
as we've seen today it's easy for "=" constraints to cause exponential solver
behavior, which is extremely painful and confusing for users.

> I worry that with this patch and our previous validation what we're really
doing
> is just training users to ignore the warnings.

I don't think there's evidence of that. In my experience, the issues we warn
about are much less common than those we don't on pub.dartlang.org. Users tend
to use the version constraints suggested by the warnings (this is where the
">=0.1.2 <0.1.3" constraints came from, for example).

https://codereview.chromium.org/173113004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/validator/dependency.dart:32:
_warnAboutNoConstraintLowerBound(dependency);
On 2014/02/20 01:22:49, Bob Nystrom wrote:
> If you know your package works with the first version of a package, it doesn't
> seem unreasonable to omit this. Sure, the package author could later publish a
> lower version, but that's very unlikely in practice.

That seems like a much narrower case than a user just not thinking to include a
lower bound. Since it can be handled by making the lowest version an explicit
lower bound, I think this warning still has value.

This is the new warning I'm least concerned with, although it's worth nothing
that a lack of a lower bound does contribute significantly to exponential
behavior in a broken package graph.

https://codereview.chromium.org/173113004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/validator/dependency.dart:90: "future\n"
On 2014/02/20 01:22:49, Bob Nystrom wrote:
> I like this, though the bold is a bit much.

I like it. In practice it's not that eye-catching, it just gives a little punch.

https://codereview.chromium.org/173113004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/validator/dependency.dart:105: 'package along with
other packages.');
On 2014/02/20 01:22:49, Bob Nystrom wrote:
> "other package that also depend on it."

That sounds like we're saying "other packages that depend on your package", when
the actual problem is "other packages that depend on the same packages as your
package".

https://codereview.chromium.org/173113004/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/version.dart (right):

https://codereview.chromium.org/173113004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/version.dart:452: bool get isAny => min == null &&
max == null;
On 2014/02/20 01:22:49, Bob Nystrom wrote:
> The previous looks utterly wrong, which implies it isn't tested. Can you add
> some tests for this?

Done.

[Bob Nystrom, 2014-02-20 18:10:20]

LGTM, but, again, we need to document this <1.0.0 semantic version policy
somewhere.

https://codereview.chromium.org/173113004/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/validator/dependency.dart (right):

https://codereview.chromium.org/173113004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/validator/dependency.dart:29:
_warnAboutSingleVersionConstraint(dependency);
On 2014/02/20 01:48:24, nweiz wrote:
> On 2014/02/20 01:22:49, Bob Nystrom wrote:
> > I think a single version constraint is potentially legitimate, at least for
> > packages <1.0.0. Since any version can have breaking changes, it may be that
> you
> > only support a single version.
> 
> That's a harmful thing to declare, though. People who care about semantic
> versions tend to use them even for pre-1.0 packages,

They may "tend" to use them (and I'm not even convinced many do), but that
behavior is unspecified by semver and not documented by us anywhere that I know
of. This is basically yelling at the user for breaking a rule they could have
had no way of knowing.

> On the other hand,
> as we've seen today it's easy for "=" constraints to cause exponential solver
> behavior, which is extremely painful and confusing for users.

Yeah, I understand that's a pain. But in practice this doesn't seem to happen
much or users would be yelling at us about the solver being too slow. I can't
recall any complaints to that effect.

> about are much less common than those we don't on http://pub.dartlang.org.
Users tend
> to use the version constraints suggested by the warnings (this is where the
> ">=0.1.2 <0.1.3" constraints came from, for example).

Yeah, I <3 the suggestions.

https://codereview.chromium.org/173113004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/validator/dependency.dart:105: 'package along with
other packages.');
On 2014/02/20 01:48:24, nweiz wrote:
> On 2014/02/20 01:22:49, Bob Nystrom wrote:
> > "other package that also depend on it."
> 
> That sounds like we're saying "other packages that depend on your package",
when
> the actual problem is "other packages that depend on the same packages as your
> package".

How about: 

An overly tight constraint makes it difficult for people to use your package
along with other packages that also depend on this one.

[nweiz, 2014-02-20 19:32:19]

https://codereview.chromium.org/173113004/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/validator/dependency.dart (right):

https://codereview.chromium.org/173113004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/validator/dependency.dart:29:
_warnAboutSingleVersionConstraint(dependency);
On 2014/02/20 18:10:21, Bob Nystrom wrote:
> On 2014/02/20 01:48:24, nweiz wrote:
> > On 2014/02/20 01:22:49, Bob Nystrom wrote:
> > > I think a single version constraint is potentially legitimate, at least
for
> > > packages <1.0.0. Since any version can have breaking changes, it may be
that
> > you
> > > only support a single version.
> > 
> > That's a harmful thing to declare, though. People who care about semantic
> > versions tend to use them even for pre-1.0 packages,
> 
> They may "tend" to use them (and I'm not even convinced many do), but that
> behavior is unspecified by semver and not documented by us anywhere that I
know
> of. This is basically yelling at the user for breaking a rule they could have
> had no way of knowing.

I don't think it's a stretch for the user to be able to figure out that an "="
constraint is likely to cause conflicts.

> > On the other hand,
> > as we've seen today it's easy for "=" constraints to cause exponential
solver
> > behavior, which is extremely painful and confusing for users.
> 
> Yeah, I understand that's a pain. But in practice this doesn't seem to happen
> much or users would be yelling at us about the solver being too slow. I can't
> recall any complaints to that effect.

Issue 16910 springs to mind. I think we're in a bit of a grace period for now
because much of the mature package ecosystem is written by the Dart team, and we
do follow this advice.

> > about are much less common than those we don't on http://pub.dartlang.org.
> Users tend
> > to use the version constraints suggested by the warnings (this is where the
> > ">=0.1.2 <0.1.3" constraints came from, for example).
> 
> Yeah, I <3 the suggestions.

https://codereview.chromium.org/173113004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/validator/dependency.dart:105: 'package along with
other packages.');
On 2014/02/20 18:10:21, Bob Nystrom wrote:
> On 2014/02/20 01:48:24, nweiz wrote:
> > On 2014/02/20 01:22:49, Bob Nystrom wrote:
> > > "other package that also depend on it."
> > 
> > That sounds like we're saying "other packages that depend on your package",
> when
> > the actual problem is "other packages that depend on the same packages as
your
> > package".
> 
> How about: 
> 
> An overly tight constraint makes it difficult for people to use your package
> along with other packages that also depend on this one.

"this one" still sounds like it's referring to your package, but if I replace
that with the name of the package that's constrained then that works.

[nweiz, 2014-02-20 19:36:00]

Committed patchset #3 manually as r32867 (presubmit successful).