dart-lang / test

A library for writing unit tests in Dart.
https://pub.dev/packages/test
496 stars 214 forks source link

revert to min sdk 2.12 #2400

Closed bsutton closed 1 year ago

bsutton commented 1 year ago

I'm writing cli apps that depend on matcher for unit tests.

My cli apps need to support users with a dart version as early as dart 2.12 (unfortunately).

With the pending release of dart 3.x and the associated deprecation of items such as CastError the matcher team has updated matcher package to remove these.

This is great (and thanks for getting this done early) but as a part of the change the matcher team has changed the min dart sdk to 2.17.

I've just pulled the code base and compiled it with dart 2.12.

There is only a single line of code that relies on 2.17

class CustomRangeError extends RangeError {
  CustomRangeError.range(
      super.invalidValue, int super.minValue, int super.maxValue)
      : super.range();

which can be reverted to:

CustomRangeError.range(
      invalidValue, int minValue, int maxValue)
      : 
      super.range(invalidValue, minValue, maxValue);

Except for this use of super. there appears to be no reason why this package needs dart 2.17.

Is there any chance we could have the min sdk reverted to 2.12 and matcher republished?

Unfortunately users of version back to 2.12 is going to linger for some time and unnecessary changes to the min sdk in packages as fundamental as matcher just make it harder to support these users.

I will raise a PR with the suggested change.

jakemac53 commented 1 year ago

Is there any reason you can't just use an older version of matcher?

devoncarew commented 1 year ago

Thanks for the feedback @bsutton (and the associated PR). I came here with the same question as @jakemac53; we are going to need to make 3.0 compatibility changes to the ecosystem packages. Generally, package:matcher evolves pretty slowly, so I'd imagine that an older version of matcher (>=0.12.0 <0.12.14 ?) would work for you?

bsutton commented 1 year ago

The last version of matcher that supports dart 2.12 is matcher 0.12.12

The changes logs for matcher 0.12.13 state


## 0.12.13

* Require Dart 2.17 or greater.
* Make `isCastError` no longer depend on the deprecated `CastError` type.
* Annotate `TypeMatcher.having` with `useResult`.

This means that to compile against dart 3.x, I must use at least matcher 0.12.13 as 3.x deprecates CastError.

However matcher 0.12.13 requires dart 2.17 and I need to support dart 2.12 through to dart 3.x inclusive.

I can't imagine that I'm the only tool developer that wants to support from 2.12 give that community polls so that a significant portion of the dart community is still using older versions of dart.

Given the ubiquity of matcher and the fact that the dart version increment was unnecessary it would seem wise to maintain backward compatibility until there is a clear reason to increment the dart version no.

jakemac53 commented 1 year ago

Ok - so the issue here is that you have a package which depends on matcher, and you want it to be able to support 2.12.0 through 3.x, but you can't do that as there is no version of matcher that allows that.

While it is a noble goal to want to support old SDKs, effectively that means that you cannot use new language features. It is best for everybody if the ecosystem just moves forward to the latest versions of the SDK, otherwise what point is there in improving the language at all, if nobody can use it?

I can't imagine that I'm the only tool developer that wants to support from 2.12 give that community polls so that a significant portion of the dart community is still using older versions of dart.

If you can provide some compelling evidence that enough people would be left behind, that would be a good place to start. I haven't personally seen any figures on this, at least not recently. cc @kevmoo might also have some numbers.

bsutton commented 1 year ago

I've got data from a Reddit poll which I will post when I get home re usage.

While it is a noble goal to want to support old SDKs, effectively that means that you cannot use new language features.

For an app I completely agree with you, for a core library not so much.

If there is a measurable benefit to the maintainers or more importantly the community then sure upgrade the SDK version.

In this case there was a one line change that provided minimal benifits to the maintainers and given my predicament a negative impact on users of the library.

On Thu, 5 Jan 2023, 9:12 am Jacob MacDonald, @.***> wrote:

Ok - so the issue here is that you have a package which depends on matcher, and you want it to be able to support 2.12.0 through 3.x, but you can't do that as there is no version of matcher that allows that.

While it is a noble goal to want to support old SDKs, effectively that means that you cannot use new language features. It is best for everybody if the ecosystem just moves forward to the latest versions of the SDK, otherwise what point is there in improving the language at all, if nobody can use it?

I can't imagine that I'm the only tool developer that wants to support from 2.12 give that community polls so that a significant portion of the dart community is still using older versions of dart.

If you can provide some compelling evidence that enough people would be left behind, that would be a good place to start. I haven't personally seen any figures on this, at least not recently. cc @kevmoo https://github.com/kevmoo might also have some numbers.

— Reply to this email directly, view it on GitHub https://github.com/dart-lang/test/issues/2400, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAG32OGZM546A6ZAFZ7RVCLWQXYUZANCNFSM6AAAAAATQKGODQ . You are receiving this because you were mentioned.Message ID: @.***>

jakemac53 commented 1 year ago

@bsutton can you not just have a constraint on matcher like >=0.12.12 <0.13.0? Users on Dart 3 will get matcher 0.12.13 when they upgrade, your package does not need to require 0.12.13 (and generally, you shouldn't be increasing min constraints of your deps just to require users to pick up certain bug fixes).

bsutton commented 1 year ago

can you not just have a constraint on matcher like >=0.12.12 <0.13.0? No, that doesn't work.

The reason it fails is that there is a secondary constraint on CLI apps (which this is).

I think the requirements of CLI and server side apps are not so well understood by the dart devs given the focus on flutter.

With a flutter app you use loose version constraints and run 'dart pub upgrade'. This creates a pubspec.lock file - locking down each package to a specific version. You then run your tests and create a release build. The release build ships with a single fixed version of each package. You can now guarantee that your flutter app will run on every system.

With a CLI app when you publish to pub.dev the pubspec.lock isn't pushed to pub.dev.

The result is that when your CLI app is installed via global activate you have no idea which version of a library it will be installed with and as such you haven't tested against all of those possible variants.

I've raised an issue against the pub repository that looks to help solve this problem: https://github.com/dart-lang/pub/issues/3668

If you want more details I've written a whole blog about it (building CLI apps is sort of my thing). https://onepub.dev/Blog?id=fvvuhnofly. For what it's worth, this is the most read blog I've ever written so it clearly is an area of interest.

This has been a source of stability issues for DCli and onepub (and any other CLI app published to pub.dev). To overcome this problem I locked down the version numbers of every package.

With dev dependencies you can in theory leave the version constraints more relaxed because they don't come into play when deployed via 'global activate'. However when I tried to relax my dev constraints version solving failed as the dev constraints depend on a number packages (e.g. collections) that are also used by the core dependencies.

As an aside, I get the impression that global activate doesn't do tree shaking as it will complain about incompatibilities in classes that aren't actually used.

generally, you shouldn't be increasing min constraints of your deps just to require users to pick up certain bug fixes

So this is good in theory, but doesn't work in practice with CLI apps, as you can't test with every possible combination of libraries that exist now and might be released in the future. This is further compounded by the fact that in the dart world (and probably every other language) you can't rely on semantic versioning.

So I guess this leads me back to the original request of getting the min sdk reverted, given the increment provides no tangible benefits and is likely to affect other CLI developers it would be really good if we can get the min sdk version reverted. It would also be good if this same principle is applied to other libraries

On Fri, Jan 6, 2023 at 4:33 AM Jacob MacDonald @.***> wrote:

@bsutton https://github.com/bsutton can you not just have a constraint on matcher like >=0.12.12 <0.13.0? Users on Dart 3 will get matcher 0.12.13 when they upgrade, your package does not need to require 0.12.13 (and generally, you shouldn't be increasing min constraints of your deps just to require users to pick up certain bug fixes).

— Reply to this email directly, view it on GitHub https://github.com/dart-lang/test/issues/2400, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAG32ODEYEGL6WUO7CO45Z3WQ4AVTANCNFSM6AAAAAATQKGODQ . You are receiving this because you were mentioned.Message ID: @.***>

jakemac53 commented 1 year ago

I think the requirements of CLI and server side apps are not so well understood by the dart devs given the focus on flutter.

Well, I maintain test and build, probably the two most popular CLI apps that are packages, so I am well aware of the constraints and challenges :).

It can be a bit more challenging with pub global activate at times (I also work on several globally activated packages), users have less recourse if it fails (they can't override deps etc). I still wouldn't pin dependencies, but I get why you might want to, but it has consequences.

The result is that when your CLI app is installed via global activate you have no idea which version of a library it will be installed with and as such you haven't tested against all of those possible variants.

This is true of all packages - whether they are cli apps or not. Nobody is testing against all combinations of all packages, and most don't even test with a pub downgrade. Semantic versioning exists to solve exactly this problem though. You should not get broken by packages unless they do major releases. If you do, that package did something wrong, outside of a few very narrow cases that we consider not breaking but technically are (adding new members to classes etc, is breaking if somebody implements the class).

Yes it takes some faith to trust that this will work, but it does work, generally. Basically the entire package ecyosystem relies on this every day.

This has been a source of stability issues for DCli and onepub (and any other CLI app published to pub.dev).

This really shouldn't be a common problem, if packages you are depending on are breaking you without major releases, you should stop using those packages. Semantic versioning takes a lot of time and care, but if a maintainer isn't willing to take the time to do it consistently then it's best to just not depend on them. Vendor your own copy of the package if you have to.

Note that pinning also has stability issues, related to exactly this issue you are filing today. Sometimes the SDK evolves, and even fairly core packages have to be updated for new sdks. If you pin deps, your users can't pick up the new versions with those bug fixes.

However when I tried to relax my dev constraints version solving failed as the dev constraints depend on a number packages (e.g. collections) that are also used by the core dependencies.

I don't see how relaxing the constraints here could cause issues? If anything it should make your version solve more likely to succeed. Pinning dependencies in general does definitely make version solving more challenging, you are more likely to get into a situation where there isn't a valid solve.

As an aside, I get the impression that global activate doesn't do tree shaking as it will complain about incompatibilities in classes that aren't actually used.

The entire app is always analyzed for errors (everything imported), in all compilation modes.

So this is good in theory, but doesn't work in practice with CLI apps, as you can't test with every possible combination of libraries that exist now and might be released in the future. This is further compounded by the fact that in the dart world (and probably every other language) you can't rely on semantic versioning.

Again, we do rely on semantic versioning all the time. If a package you are depending on isn't respecting it, file issues on them asking them to, and if they refuse don't use the package. No dart-lang packages pin dependencies and it doesn't cause problems (yes, we own all our deps, so its cheating a bit, but the whole ecosystem breaks down if packages start pinning deps).

So I guess this leads me back to the original request of getting the min sdk reverted, given the increment provides no tangible benefits and is likely to affect other CLI developers it would be really good if we can get the min sdk version reverted.

I really don't see any convincing reason to do this. You have options - you could even just use a version range for package:matcher and continue pinning other deps. That could be tight version range to a known set of versions if you want to be extra careful.

It would also be good if this same principle is applied to other libraries - only increment the min sdk version if there is a tangible benefit to developers and community or there is no choice.

Packages are free to use new language features, the old versions are still out there for anybody who is on an old SDK. They still work just fine on those old SDKs.

We have also in the past back ported critical bug fixes, if/when they come up, to keep those users functional on old versions.

bsutton commented 1 year ago

Well, I maintain test and build, probably the two most popular CLI

There is a very big difference between shipping a CLI app that lives in the pub.dev package eco system and shipping a CLI app with the Dart SDK or one which only uses internally developed google packages.

Because I ship a toolkit designed for building CLI apps I'm interacting with a large community of CLI app developers. I think I have something like a dozen CLI apps published on pub.dev, maintain 40-50 internally and help DClI users on a regular basis with their CLI app problems.

Sorry, this isn't meant to be dick swinging, just want to ensure that you understand that I have a credible amount of experience building CLI apps in the pub.dev eco system. My statements below are from a lived experience.

This really shouldn't be a common problem,

You right it shouldn't be, but the learned reality is that it is.

[semantic versioning] Basically the entire package ecyosystem relies on this every day. No it doesn't.

The whole point of pubspec.lock file is to pin package versions so that when developing a flutter app you can be certain that what you test against is what you release.

Semantic version is used on the first 'pub get' and then we rely on pubspec.lock to ensure stability. It exists because the dart developers recognised that an unstable set of dependencies is untenable.

and if they refuse don't use the package

This is essentially a completely impractical and unnecessary solution.

I don't have the resources of google and neither do most other developers.

This solution is also not necessary if as per https://github.com/dart-lang/pub/issues/3668 we can pin packages just like we do when we release a flutter app.

Vendor your own copy

I've done this on a number of occasions but I cringe every time I do it. This bifurcates the package eco system and pollutes it with unmaintained packages. I only ever vendor a package as a temporary measure (and only in an emergency) as I believe the correct process is to submit a patch and assist the maintainer in releasing a new version. Vendoring (for public packages) should only ever be suggested as a last resort.

Note that pinning also has stability issues

Pinning my apps, like releasing a flutter app does, ensures my apps stability. But just like when android releases a new version, when Dart releases a new version I expect to have to retest (which is why we are here today).

This is not the same as having to test against every new version and every possible combination of every dependency.

yes, we own all our deps, so its cheating a bit

I believe this is exactly the cause of the misunderstanding. Google developers work in a clean room, I'm out playing in the wild west.

but the whole ecosystem breaks down if packages start pinning deps

No, this isn't correct. Libraries should never pin packages, as you correctly state, it would break the entire eco systems.

CLI apps pinning packages (as do flutter apps) has no effect on the eco system as no other package relies on a CLI app.

My proposal https://github.com/dart-lang/pub/issues/3668 notes that the published pubspec.lock should only apply to the activation of the CLI app not to any library code that also ships (DCli is an example of an package that ships both a CLI app and a library intended for third party usage).

Packages are free to use new language feature

yes the are. But in every language eco system I've worked with package maintainers (particularity of core libraries) work hard to make it easy for their users and this includes not adding unnecessary constraints on their libraries.

This was an unnecessary change that provided no tangible benefit.

We have also in the past back ported critical bug fixes,

Changing the sdk version may force us to do this again with matcher. If we simply revert the sdk constraint then we won't need to do this in the future.
Again, incrementing the sdk constraint provided no benefits and exposes the team to future work that could be avoided.

I can't see any argument that suggests changing the constraint at this point in time was a good idea.

FWIW: I'm trying to make this point not just about matcher but about how google maintains all of its core libraries.

Careful consideration should always be given before a min sdk constraint is changed.

On the subject of pinning, let's try to think about it in a different way.

What would the reaction be from the flutter community be if we said:

Hey we've got this new release process. When your users installs a flutter app to their phone, we resolve the package dependencies at that point in time. This is great because if another flutter app is already installed, the two can share a package. This way your release bundles are smaller Your users get the benefit of the latest version of any package, even after you release

At this point your users would remind you of a thing call 'Windows DLL Hell' and there would be a mass exodus of flutter developers.

Currently, with respect to CLI apps, the Dart eco system is subjecting developers to DLL Hell.

This was a lesson learned a long time ago but which now seems to have been forgotten.

Edit: formatting

jakemac53 commented 1 year ago

As stated in my previous comments I do understand why you want to pin deps, I agree it will make your tool more stable.

What I disagree with is that your choice to do that, should be able to force requirements onto your dependencies. It is absolutely inevitable (especially with Dart 3.0), that you are going to be required to bump your min SDK soon, anyways. Packages are going to want to use the records feature, other packages will want to use super params, etc.

It is also important to note that we are not dropping support for 2.12 here. The old versions of the package still exist and work fine. For projects that are on an old SDK, they should expect to get stuck on old versions of packages at some point.

I would also be curious to understand why any large number of users would be stuck on 2.12. There really hasn't been much churn in terms of breaking changes on the Dart end of things, is there something in newer versions of flutter that is holding people back?

natebosch commented 1 year ago

This is a long discussion thread, but if I understand correctly, you'd like us to downgrade our language version to satisfy package authors with a particular set of goals:

I don't think this is a big enough audience to let it impose restrictions on what language features we use in core packages. I'm going to close the thread for now since we don't plan to take action.

I think the requirements of CLI and server side apps are not so well understood by the dart devs given the focus on flutter.

It's not a use case that we are targeting heavily with our tooling support. That does not imply it is a use case we do not understand.

and I need to support dart 2.12 through to dart 3.x inclusive.

Why is it mandatory that you combine a wide SDK range with pinned packages? Why do you trust changes in the SDK more than you trust changes in matcher?

If we grant the argument about the SDK not imposing a combinatorics problem, what is the argument for needing to support back to the 2.12 SDK with new releases of your package? Can you ask users who want new features, to occasionally upgrade their SDK?

and is likely to affect other CLI developers

This is very unlikely. Dependencies to matcher instead of test are rare, and not the primary target use case.

This was an unnecessary change that provided no tangible benefit.

Maintaining a bleeding edge lint set and modeling the latest best practices is considered a benefit. Letting developers in this package practice google's code hygiene practices and encouraging frequent cleanup is also considered a benefit.

Allowing transitive dependencies of this package to use the latest language version is also a benefit. If we restrict this package to maintain backwards compatibility to 2.12.0 then we'd have to restrict it's deps too. This package will soon pick up a dependency on package:test_api and compatibility with the 2.12.0 SDK will not be feasible anymore, it wouldn't be a matter of changing 1 usage of a new feature. The test package will then become only compatible with that version of matcher, so even if we try to make an accommodation for this case it's certainly going to become incompatible again for anyone on the latest version of test.

I don't know that we've ever written it out, but I think the general philosophy we've been using (cc @devoncarew is there anywhere we should document this? @kevmoo @jakemac53 does this match your understanding?):