Skip to content
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

Packages with example apps opted-in for null safety fail the null safety checks #44013

Closed
amirh opened this issue Oct 31, 2020 · 14 comments
Closed
Assignees
Labels
area-language Dart language related items (some items might be better tracked at github.com/dart-lang/language). NNBD Issues related to NNBD Release P1 A high priority bug; for example, a single project is unusable or has many test failures

Comments

@amirh
Copy link

amirh commented Oct 31, 2020

This came up in flutter/flutter#69440 (comment) where a bunch of flutter/plugins packages started failing on CI.

@zanderso and @leafpetersen pointed out that the failing packages included example apps with no Dart SDK constraints which means they are treated as opted-in for null safety.

While the flutter/plugins packages are fixed in flutter/plugins#3240 it's somewhat worrying that many other packages in the ecosystem will take a similar hit.

I counted the affected packages across pub and there are currently 725 packages with example apps that are opted-in for null safety (most likely unintentionally as null safety isn't launched yet).

698 of these packages are actually not opted-in for null safety (e.g the root pubspec.yaml isn't opted-in while example/pubspec.yaml is opted in).
For these 698 packages I believe it should be safe to assume that the sdk constraint in example/pubspec.yaml should be intersected with the sdk constraints of the root pubspec.yaml. So it's fairly easy to automate the fix.

The list of the 698 "fixable" packages is available in this sheet.

I'm happy to send PRs using dart_lsc to fix example/pubspec.yaml for as many of these 698 packages as possible.
I guess another alternative to consider would be to somehow teach the analyzer that the example app's sdk constraint cannot be wider than the package's sdk constraint.

cc @mit-mit @csells

@mit-mit
Copy link
Member

mit-mit commented Nov 2, 2020

The root cause here is that when a project doesn't have an SDK lower bound, no language version is written into .dart_tool/package_config.json.

@mit-mit
Copy link
Member

mit-mit commented Nov 2, 2020

Jonas logged https://github.com/dart-lang/pub-dev/issues/4208 for a potential pub change. Please thumbs up/down/comment there for that proposal.

@devoncarew devoncarew added the area-language Dart language related items (some items might be better tracked at github.com/dart-lang/language). label Nov 2, 2020
@leafpetersen
Copy link
Member

We have reports of users downstream of packages that have no lower sdk constraint getting broken on master. For example, these two packages are broken on master. I see two options here:

  • Manually fix up the broken packages on pub (may be difficult to get PRs landed)
  • Change the interpretation of missing lower bounds to mean "2.7" instead of "latest".

I think we likely need to do one or the other for beta. cc @franklinyow @jonasfj @natebosch @lrhn @jakemac53 @munificent

@franklinyow franklinyow added NNBD Issues related to NNBD Release P1 A high priority bug; for example, a single project is unusable or has many test failures labels Nov 10, 2020
@franklinyow
Copy link
Contributor

@leafpetersen
Are you saying that we need additional work in additional to dart-lang/pub#2707?

@amirh
Copy link
Author

amirh commented Nov 10, 2020

@mit-mit mit-mit added this to Should target beta in Null safety experience issues Nov 11, 2020
@mit-mit
Copy link
Member

mit-mit commented Nov 11, 2020

Discussed in language team, and agreed that:

  • Keep the current ERROR when resolving a pubspec that itself is missing one or more parts of the upper/lower Dart SDK constraint
  • Add new logic in pub get that for dependencies which do not have a lower constraint in their pubspec, defaults the language version to 2.7, and write that to the package config

@leafpetersen
Copy link
Member

@mit-mit
Copy link
Member

mit-mit commented Nov 12, 2020

Pub was updated in dart-lang/pub#2748

@jonasfj
Copy link
Member

jonasfj commented Nov 12, 2020

@jonasfj
Copy link
Member

jonasfj commented Nov 12, 2020

@pcsosinski @franklinyow, have beta been cut yet? If so we would like to cherry-pick: 4b3840d
(I can't seem to find a branch that looks like it is beta for 2.12 -- maybe I'm just missing something obvious :D)

@jonasfj
Copy link
Member

jonasfj commented Nov 12, 2020

Filed cherrypick request: #44176

@timsneath
Copy link
Contributor

@mit-mit Will this behavior (default for packages missing SDK constraint) be documented somewhere on dart.dev?

@mit-mit
Copy link
Member

mit-mit commented Nov 13, 2020

We weren't planning on documenting this particular change. Note that we didn't change the behavoir for running pub get in a directory with a pubspec not containing an SDK constraint. That continues to be an error (as described in the breaking change).

But this makes a small tweak to the behavoir when you run pub get on a pubspec that contains a dependency, and the pubspec of that dependency doesn't have an SDK constraint. In that case, we default the dependency to be 2.7. The owner of the dependency will get the error when they pub get, so they already have the needed message (and docs for that).

dart-bot pushed a commit that referenced this issue Nov 13, 2020
New in this revision:

> git log --format="%C(auto) %h %s" b10966c6a8ad7d95c2023b7842fa2697001d2fdf..c00d4b4abf5b4ff265a7ce6282b748551f1b5b1f
 c00d4b4a Improve message in `pub outdated` when incompatible versions are found (#2746)
 2a177623 outdated --mode=null-safety, resolvable constrained to null-safe vers… (#2739)
 3ea2b832 Default language version (#2748)
 78865460 Don't allow both --json and --transitive (#2742)

Fixes #44013

Change-Id: I0dc876c5e57777a4724d90662408f91910e2517c
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/171720
Commit-Queue: Sigurd Meldgaard <sigurdm@google.com>
Reviewed-by: Jonas Jensen <jonasfj@google.com>
Reviewed-by: Michael Thomsen <mit@google.com>
@leafpetersen
Copy link
Member

We weren't planning on documenting this particular change

Agreed. This change just makes existing packages continue to work, all new packages will be required to have an sdk constraint.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-language Dart language related items (some items might be better tracked at github.com/dart-lang/language). NNBD Issues related to NNBD Release P1 A high priority bug; for example, a single project is unusable or has many test failures
Projects
Development

No branches or pull requests

7 participants