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

Drop support for unsound null safety in Dart 3 #3424

Merged
merged 15 commits into from
Jan 18, 2023
Merged

Drop support for unsound null safety in Dart 3 #3424

merged 15 commits into from
Jan 18, 2023

Conversation

jakemac53
Copy link
Contributor

Note: This will be broken until the ddc resources move dart-lang/sdk#50700

Remove entirely the build_vm_compilers package, this should be marked discontinued on pub.

Drop all code relating to sound/unsound null safety.

Update build_web_compilers, build_vm_compilers, and build_runner to only
support 3.x sdks.

Point at the new DDC resources in the SDK based on dart-lang/sdk#50700.

Remove entirely the build_vm_compilers package, this should be marked
discontinued on pub.

Drop all code relating to sound/unsound null safety.

Update build_web_compilers, build_vm_compilers, and build_runner to only
support 3.x sdks.
@jakemac53 jakemac53 marked this pull request as draft December 13, 2022 22:14
@jakemac53
Copy link
Contributor Author

cc @nshahan

@jakemac53
Copy link
Contributor Author

jakemac53 commented Dec 13, 2022

cc @simolus3 it looks like drift does still use build_vm_compilers. We are looking at probably discontinuing that package, would that be an issue for you, or is it more legacy? What are you using it for? (note that package:test now uses the frontend_server compiler which is likely faster for vm tests than build_vm_compilers + build_runner test)

@simolus3
Copy link
Contributor

I don't think drift is using build_vm_compilers at all - I also couldn't find a dependency to it, I'll take another look tomorrow.

But I think the community tests are failing because they're running on Dart 2.18.5, which can't run build_runner because it now has a@dart=3.0 language version passed down from its updated pubspec.

@jakemac53
Copy link
Contributor Author

jakemac53 commented Dec 13, 2022

Ah yeah I think it was just the way the tool works it would end up adding in that dependency even if it wasn't previously there. But it no longer overrides that package, so we see the new errors.

@jakemac53 jakemac53 marked this pull request as ready for review January 4, 2023 21:45
@jakemac53
Copy link
Contributor Author

Sending out for review, DDC changes should land soonish.

@jakemac53
Copy link
Contributor Author

@simolus3 it looks like you have a test which isn't opted in yet in drift (test/engines/delegated_database_test.dart). Is there a preferred solution on your end for getting our bots here green? I think we could just skip it (remove it from the preset maybe), or it could be migrated, or open to other suggestions also.

build_modules/pubspec.yaml Outdated Show resolved Hide resolved
build_runner/pubspec.yaml Outdated Show resolved Hide resolved
build_web_compilers/pubspec.yaml Outdated Show resolved Hide resolved
@simolus3
Copy link
Contributor

simolus3 commented Jan 13, 2023

there a preferred solution on your end for getting our bots here green?

Sorry for the delayed response, I've missed the comment. As far as I'm concerned, we can temporarily skip the community tests. I will test drift with the latest build runner on a 3.0 dev SDK as soon as this PR is merged. As soon as everything works (it may very well be that drift tests fail in 3.0 for reasons unrelated to build), I can follow up with a PR to re-enable the tests.

I have migrated all tests in that repository to null-safety now, but not on the branch used by the community tests. You could try changing the ref to develop here, but I wouldn't waste any time on it if that doesn't fix the problem. I can take the time later to test with a dev SDK and follow up with a fix.

ref: latest-release

@jakemac53
Copy link
Contributor Author

@simolus3 thanks, I tried that branch but got errors in the dependency solve, so just commenting out the community jobs for now, happy to restore them once they are working again

@jakemac53 jakemac53 merged commit 82889ae into master Jan 18, 2023
@jakemac53 jakemac53 deleted the dart-3 branch January 18, 2023 17:25
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

Successfully merging this pull request may close these issues.

3 participants