Skip to content

Migrate dart:io and its tests to NNBD #40040

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.

Sign up for GitHub

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

Jump to bottom
Closed
leafpetersen opened this issue Jan 9, 2020 · 18 comments
Closed

Migrate dart:io and its tests to NNBD #40040

leafpetersen opened this issue Jan 9, 2020 · 18 comments
Assignees
@sortie
Labels
area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. NNBD Issues related to NNBD Release vm-nnbd-unfork-sdk Label for all issues that need to be done before the nnbd sdk can be unforked
Milestone
D28 Release

Comments

@leafpetersen
Copy link
Member

This tracks the migration of dart:io and related tests.
@leafpetersen leafpetersen added NNBD Issues related to NNBD Release area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. labels Jan 9, 2020
@leafpetersen
Copy link
Member Author

Migration instructions here.

@sortie
Copy link
Contributor

sortie commented Jan 10, 2020

So far I've migrated 11 of 30 files in sdk_nnbd/lib/io.

@a-siva a-siva added the vm-nnbd-unfork-sdk Label for all issues that need to be done before the nnbd sdk can be unforked label Jan 16, 2020
@a-siva a-siva mentioned this issue Jan 16, 2020
Closed
@sortie
Copy link
Contributor

sortie commented Jan 20, 2020

I've now migrated 88% (13619 of 15393) of the lines in dart:io and its VM patch files.

@sortie
Copy link
Contributor

sortie commented Jan 21, 2020

I've now completed a first pass of 100% of the lines in dart:io and its VM patch files. I'm taking a second look at my diff and discussed issues with the language team. I'll revise my changes and then upload them for review. I was not able to build the VM with it turned because the other core library patch files have not been migrated yet. The dart2js stub dart:io patch files also needs to be migrated, I might as well do that while here.

I'm yet to begin migrating the dart:io tests. The first pass will be to attempt fixing the compile time errors with the analyzer and the migration tool. The second pass will be running the tests, but that is blocked on the dart:io dependencies (such as core and others) having their patch files migrated, so the NNBD SDK can be built with dart:io NNBD turned on.

@leafpetersen
Copy link
Member Author

Sounds great! Are you planning to land the library code and then follow up with the tests, or do you plan to do them all in one?

@sortie
Copy link
Contributor

sortie commented Jan 21, 2020

I will land the library and patch file changes first since they are bundled and make sense on their own and does not need to wait for testing (which I don't think would work yet due to other missing patch files). The tests will be done in their own phase.

@leafpetersen
Copy link
Member Author

👍

@franklinyow franklinyow added this to the D28 Release milestone Jan 21, 2020
@sortie
Copy link
Contributor

sortie commented Jan 23, 2020

I've migrated dart:io to NNBD including the patch files and uploaded the changelist for review at https://dart-review.googlesource.com/c/sdk/+/133060.

@sortie
Copy link
Contributor

sortie commented Jan 27, 2020

I have addressed almost all of the review comments locally. There's a few more complicated cases I need to finish. I'll finish those tomorrow and do a final round of review.

@sortie
Copy link
Contributor

sortie commented Jan 28, 2020

I addressed the review comments and finished up the last complicated cases. I'm now waiting on a second round of reviewing and then we should be able to land the dart:io migration.

@a-siva a-siva mentioned this issue Jan 30, 2020
Closed
dart-bot pushed a commit that referenced this issue Jan 30, 2020
@sortie
[nnbd] Migrate dart:io to NNBD.
f6e0200 
The Process class will now throw a StateError if the process is detached
upon accessing the stdin, stdout, stderr, and exitCode getters.

The Socket class will now throw a SocketException if the socket has been
destroyed or upgraded to a secure socket upon setting or getting socket
options.

Bug: #40040
Change-Id: I68e22873932c68a3fac549c0f742dd49d0a60dfb
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/133060
Commit-Queue: Jonas Termansen <[email protected]>
Reviewed-by: Lasse R.H. Nielsen <[email protected]>
Reviewed-by: Erik Ernst <[email protected]>
@sortie
Copy link
Contributor

sortie commented Jan 30, 2020

I've now migrated dart:io. It's not opted in by default yet, like most other VM libraries.

I'll start work on migrating the standalone_2/io tests.

@sortie
Copy link
Contributor

sortie commented Feb 3, 2020 •
edited
Loading

I'm migrating the standalone_2/io tests to NNBD. 181 files were already valid under NNBD. I've now migrated 26 of 85 files (30%) that needed to be migrated.

@sortie
Copy link
Contributor

sortie commented Feb 3, 2020

cc @athomas

@sortie
Copy link
Contributor

sortie commented Feb 4, 2020

I've now migrated all the tests in standalone_2/io to NNBD. I'll submit a changelist for review tomorrow.

@sortie
Copy link
Contributor

sortie commented Feb 5, 2020

I've uploaded the standalone/io migration for review:

@sortie
Copy link
Contributor

sortie commented Feb 6, 2020

Some follow up CLs to fix the final analyzer errors left in dart:io and dart:_http:

I'm still waiting for the migrated standalone/io tests to be reviewed.

@sortie
Copy link
Contributor

sortie commented Feb 6, 2020

Breaking change #40483 needs to be made in order to nicely migrate Process to NNBD.

dart-bot pushed a commit that referenced this issue Feb 7, 2020
@sortie
[nnbd] Fix Process.stderr providing stdout instead in NNBD dart:io.
7704b83 
Bug: #40040
Change-Id: Ia318180de1b1614502671c6bf4051d79ae7808f3
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/134327
Reviewed-by: Lasse R.H. Nielsen <[email protected]>
Commit-Queue: Jonas Termansen <[email protected]>
dart-bot pushed a commit that referenced this issue Feb 7, 2020
@sortie
[nnbd] Fix SecureServerSocket not allowing null SecurityContext.
514a8d4 
Bug: #40040
Change-Id: I030f3f6740fd0ffefaa20fac1be69fb74c5cbff5
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/134293
Reviewed-by: Lasse R.H. Nielsen <[email protected]>
Commit-Queue: Jonas Termansen <[email protected]>
dart-bot pushed a commit that referenced this issue Feb 7, 2020
@sortie
[nnbd] Fix remaining NNBD errors in dart:_http and dart:io.
02ced42 
HeaderValue parameters must have non-null values per the specification.

Bug: #40040
Change-Id: I39f8c894015197c014057888026e4ad474aa5dc8
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/134722
Reviewed-by: Lasse R.H. Nielsen <[email protected]>
Commit-Queue: Jonas Termansen <[email protected]>
dart-bot pushed a commit that referenced this issue Feb 7, 2020
@sortie
[nnbd] Migrate standalone/io to NNBD.
3433d8c 
The tests are also now dartfmt.

Bug: #40040
Change-Id: I8dece8097b37b70d47a5374dae2f3fadb0fc4b90
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/134338
Commit-Queue: Jonas Termansen <[email protected]>
Reviewed-by: Bob Nystrom <[email protected]>
@sortie
Copy link
Contributor

sortie commented Feb 7, 2020

I've now landed all the CLs migrating dart:io to NNBD (including fixing the remaining errors in dart:_http).

I've also just landed the migration of the standalone/io tests to NNBD.

That's should be it all, barring actually getting the tests working later on. Closing.

@sortie sortie closed this as completed Feb 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@sortie sortie

Labels
area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. NNBD Issues related to NNBD Release vm-nnbd-unfork-sdk Label for all issues that need to be done before the nnbd sdk can be unforked
Projects
None yet
Milestone
D28 Release
Development

No branches or pull requests
4 participants
@sortie @leafpetersen @a-siva @franklinyow