Skip to content

Pre-release SDK constraints should not require a pre-release version #3897

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
grouma opened this issue Jul 13, 2020 · 30 comments
Closed

Pre-release SDK constraints should not require a pre-release version #3897

grouma opened this issue Jul 13, 2020 · 30 comments

Comments

@grouma
Copy link
Member

grouma commented Jul 13, 2020

The following error should only be a warning:

Packages with an SDK constraint on a pre-release of the Dart SDK should themselves be published as a pre-release version.

See this specific example: dart-lang/build#2753

@jakemac53
Copy link
Contributor

jakemac53 commented Jul 13, 2020

This should be a warning only, otherwise breaking changes in dev sdk's cannot be fixed for users without them explicitly updating to dev releases of their (possibly transitive) dependencies.

Pre-release packages are deprioritized in pub solves so you will end up with only the stable version by default.

There are also other situations where you know it is safe to depend on something from a dev sdk (see the linked example).

@grouma
Copy link
Member Author

grouma commented Jul 13, 2020

Note this is completely blocking. Dropping back to an older SDK (2.7.0) still has this issue. Dropping back further requires a now optional author field.

This issue appears to even be in version 1.24 :/

@grouma
Copy link
Member Author

grouma commented Jul 13, 2020

Looks like this is actually server side: #3607

@jakemac53
Copy link
Contributor

jakemac53 commented Jul 14, 2020

Note that this is also not going to work nicely with our NNBD roll-out strategy. We want to have the core ecosystem packages (at least those required to run tests) published and ready to go as soon as nnbd is released. The strategy of releasing those packages as non-breaking versions is heavily reliant upon them being available the first time any user on the new sdk runs pub upgrade (although yes, there is no guarantee users will actually be able to get those versions anyways).

We could possibly publish packages with a stable sdk constraint on an SDK which doesn't yet exist (no idea if pub allows this or not). At a minimum that would tank the pub score of the packages.

Otherwise we would be stuck having to scramble to publish these packages once the sdk does land, which we can't realistically do in as quick of a timeframe as we would like.

We would instead prefer to add a constraint like >=2.10.0-0 <2.11.0 (which allows both the 2.10 dev sdk as well as stable). We would only do that once dev is in fact stabilized (possibly after cutting the stable branch).

cc @leafpetersen

@jonasfj
Copy link
Member

jonasfj commented Jul 21, 2020

This was discussed in #3557.

I don't feel particularly strongly about it. The fact that we don't enforce the transitive variant of this property makes it seem less useful to me.

On the other hand, a stable SDK release should not break packages. If it does this is liable to cause pain because our SDK upper-bound constraints are wrong. That said, I see how it's sometimes necessary and reasonable to work around.

NB. if we decide to change this, we can easily fix it on the server.

@jakemac53
Copy link
Contributor

I guess I missed the response to my concerns in the original discussion there :/.

@sigurdm
Copy link
Contributor

sigurdm commented Jul 28, 2020

cc @mit-mit surely wants to weigh in here.

@leafpetersen
Copy link
Member

I'm pretty concerned about making this an error. We're having a lot of problems right now with our processes already requiring too much atomicity, and I'm quite worried that this might exacerbate the problem.

@jakemac53
Copy link
Contributor

jakemac53 commented Jul 28, 2020

Consider also the following scenario for any package depended on by flutter:

  1. The SDK releases some new feature that a package wants to use.
  2. The package starts using it, and has to stay at a pre-release version.
  3. We start preparing the stable flutter/dart sdks, and want to update to the latest version of said package in flutter.
  • We cannot publish a stable version of the package because it depends on a dev SDK, and the stable SDK is not yet released
  • At the same time flutter stable should really not be pinning dev releases of packages (null safety being the exception right now, but not likely a strategy we want to repeat)

How do we resolve this scenario? Do you have to wait two stable releases before using any new sdk feature in a package depended on by flutter?

@grouma
Copy link
Member Author

grouma commented Jul 28, 2020

Consider also the following scenario for any package depended on by flutter:

The SDK releases some new feature that a package wants to use.

This is the exact scenario that caused issues outside of Flutter. The Dart SDK began writing metadata files that we wanted to consume from package:dwds. It blocked us from publishing a non dev version of package:webdev.

@nshahan
Copy link

nshahan commented Jul 29, 2020

As another data point for the discussion, this issue is blocking publishing a new version of the package dart_internal that works with the sdk v2.9.0. 9 of the 10 published versions of the package have already been published with a lower sdk bound of 2.0.0-dev.12.0. Should I have to arbitrarily bump the lower bound to publish?

See the history of commits to the pubspec: https://github.com/dart-lang/sdk/commits/master/pkg/dart_internal/pubspec.yaml

@jakemac53
Copy link
Contributor

jakemac53 commented Jul 31, 2020

This is now blocking us from fixing a real issue, dart-lang/build#2763

All users will have to pin analyzer or build_runner on the flutter release that corresponds to version 2.10, and we cannot publish a fix that does not break users of 2.9 because of this arbitrary restriction.

@leafpetersen
Copy link
Member

Jake's comment should be "this is now blocking".

cc @athomas @jonasfj @sigurdm we really need to revert this change (or make it a warning) and reconsider at leisure. Are any of you around to do this on Friday (AAR time) so that we can land the fixes early Friday morning SEA time?

@jonasfj
Copy link
Member

jonasfj commented Jul 31, 2020 via email

@jakemac53
Copy link
Contributor

that was... quite the typo 😆 - fixed

@athomas athomas transferred this issue from dart-lang/pub Jul 31, 2020
@athomas
Copy link
Member

athomas commented Jul 31, 2020

@sigurdm and @jonasfj are OOO.

Given that these issues seem connected to the stable release that is slated to go out on Monday, I have asked @isoos and @sortie to prepare for deploying a revert.

@leafpetersen
Copy link
Member

Thanks @athomas ! @jakemac53 are you unblocked from landing these fixes now?

@sortie
Copy link

sortie commented Jul 31, 2020

I am currently deploying them to production. It should be fully up in about an hour.

@sortie
Copy link

sortie commented Jul 31, 2020

The traffic has been switched on pub.

@sigurdm
Copy link
Contributor

sigurdm commented Aug 3, 2020

I guess this means we have to implement the warning in the client?

@isoos
Copy link
Collaborator

isoos commented Aug 3, 2020

I guess this means we have to implement the warning in the client?

We could also keep the rule and have a case-by-case opt out.

@mit-mit
Copy link
Member

mit-mit commented Aug 3, 2020

I browsed though this thread, and the linked bugs, but I still don't understand why we need a change.

I still believe this needs to be an error: A package which depends on a pre-release package is itself pre-release and this should be versioned as such.

What is the canonical use case where this is intractable?

@mit-mit
Copy link
Member

mit-mit commented Aug 3, 2020

Consider also the following scenario for any package depended on by flutter:

  1. The SDK releases some new feature that a package wants to use.

This is in a non-stable SDK, right?

  1. The package starts using it, and has to stay at a pre-release version.

Yes, it definitely should. It's using a feature which has no stability promise and could be removed at any point in time.

  1. We start preparing the stable flutter/dart sdks, and want to update to the latest version of said package in flutter.

We cannot publish a stable version of the package because it depends on a dev SDK, and the stable SDK is not yet released

Yup, sounds correct.

At the same time flutter stable should really not be pinning dev releases of packages (null safety being the exception right now, but not likely a strategy we want to repeat)
How do we resolve this scenario? Do you have to wait two stable releases before using any new sdk feature in a package depended on by flutter?

Why can't we publish the stable version of the package that depends on the about-to-be-stable SDK? It would not be resolvable until the stable SDK ships, but that's the behavoir we want, isn't it?

@jakemac53
Copy link
Contributor

jakemac53 commented Aug 3, 2020

Regardless about how you feel about depending on features that only exist in a dev SDK, there are other reasons that you may need to depend on a dev SDK in a stable release which we can't predict.

This is exactly what happened here - we needed people to get a different version of build_runner depending on their language version (ie: major/minor sdk version), otherwise they would be broken (and were actively broken). This needed to be a stable release of build_runner because otherwise most people would not get it (they would instead get some older version). We had to choose between breaking our flutter stable users or our flutter master/dev/beta users, and ended up choosing to break the non-stable users until we could publish a version for them.

In the original design document where we agreed not to make this an error, I called out another possible scenario where the SDK could make a breaking change (this happens relatively often). You might need to release a version of your package with a backwards incompatible fix, so it would need to set the min version to a dev version of the SDK, but you would need all users on that sdk to pick it up, so a stable version release is the correct solution to fix users on the dev channel.

We can't possibly understand ahead of time all the reasons people might have for needing to have a dev constraint on the SDK. The bar for blocking publishing of packages without any workaround needs to be a lot higher than a concern about features being reverted, especially when it actually blocks us from fixing real world, concrete breakage.

Yes, it definitely should. It's using a feature which has no stability promise and could be removed at any point in time.

I disagree with this assessment. Any breakage that could occur would happen during the pre-release window, and will only break users who have already opted into some level of breakage by being on the pre-release sdk.

There is a lot of value in having these users pick up versions of packages that actually use the features provided by their pre-release sdk. Isn't this in fact the primary reason for having a pre-release sdk channel at all?

Why can't we publish the stable version of the package that depends on the about-to-be-stable SDK? It would not be resolvable until the stable SDK ships, but that's the behavoir we want, isn't it?

This would break flutter and put it in a state where it can't be tested because its dependencies aren't valid. Beyond that, this is a strictly worse solution than allowing us to publish versions that depend on a pre-release. You are effectively doing exactly the same thing, except you can't even run your code to validate it works.

See also my concern re: NNBD releases. We want users to pick up these packages before the stable null safety release, during the stabilization period. This will lead to a much better experience for users since many of them will already have the null safe dependencies the first time they run on the new sdk (whether they use pub get or pub upgrade).

@jonasfj
Copy link
Member

jonasfj commented Aug 3, 2020

This would break flutter and put it in a state where it can't be tested because its dependencies aren't valid.

We could publish the same code as pre-release and stable release, only bumping the version number.
But we don't have tooling to do this automatically, nor is it impossible that simply changing the version number can break a package (it shouldn't but people do embed these in code and sometimes tests -- mostly seen in pana).

@jakemac53
Copy link
Contributor

We could publish the same code as pre-release and stable release, only bumping the version number.

Flutter pins a single version for all its deps so we can't put a version range that allows both 🤷‍♂️

I would also like to re-iterate that publishing a version of a package which claims support for a non-existent sdk is strictly less safe than just allowing publishing of stable versions that require some pre-release sdk version. At least with the latter you can validate the code and not have broken travis jobs etc.

@natebosch
Copy link
Member

natebosch commented Aug 3, 2020

Yes, it definitely should. It's using a feature which has no stability promise and could be removed at any point in time.

In this particular case the "feature" was "the language version becomes 2.10" which, if removed from the stable SDK release, would be an enormous problem.

There are many reasons we publish packages with more recent SDKs. Sometimes it's a bug fix, sometimes like above it's about fixing versioning issues unrelated to specific SDK features.

I feel strongly that this is an area where authors want to do the right thing and we only need to help them from making easy mistakes.

It would not be resolvable until the stable SDK ships, but that's the behavoir we want, isn't it?

No, generally if we wanted that behavior we'd publish when the SDK ships. When we publish with a pre-release constraint it is because we want folks to pick it up, or because the folks already on that SDK need to pick it up.

I worry that the unusual patterns of null safety around SDK and package releases has made us lose sight of the years of experience we have doing this already... This is something that we know is both necessary and safe, because we have needed to do this and we have done it safely.

@jonasfj
Copy link
Member

jonasfj commented Aug 5, 2020

I would also like to re-iterate that publishing a version of a package which claims support for a non-existent sdk is strictly less safe than just allowing publishing of stable versions that require some pre-release sdk version.

Yes, there are plenty of other pitfalls.. You can also publish a stable version that depends on a prerelease of some other package, and thus, use transitive dependencies to achieve a stable version that requires a prerelease SDK.

@jonasfj
Copy link
Member

jonasfj commented Aug 6, 2020

Closed as resolved, we're going to only keep this a warning.

@gmpassos
Copy link

Definitely a package that depends on a pre-release should be blocked, unless published as a pre-release, since the main idea to publish something is for the public, and the public won't be able to use the package normally.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests