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

Cleanup DDC files copied to the released SDK for Dart 3.0 #50700

Closed
nshahan opened this issue Dec 13, 2022 · 18 comments
Closed

Cleanup DDC files copied to the released SDK for Dart 3.0 #50700

nshahan opened this issue Dec 13, 2022 · 18 comments
Assignees
Labels
area-web Use area-web for Dart web related issues, including the DDC and dart2js compilers and JS interop. P2 A bug or feature request we're likely to work on web-dev-compiler

Comments

@nshahan
Copy link
Contributor

nshahan commented Dec 13, 2022

I want to highlight and coordinate some technically observable breaking changes to the files packaged with the SDK that I want to make for Dart 3.0. My intention is that no user (using a supported DDC build) will notice any changes. The intent is to only change the files copied to the packaged SDK that we ship externally, not the files that we build on our test bots or in google3.

With the removal of weak null safety we no longer need to ship the precompiled weak .dill files for the SDK libraries. In the process I would also like to cleanup the names of the sound null safe .dill files shipped for DDC. This will make the names consistent across dart2js, dart2wasm, and ddc.

@mit-mit

  • I'm not sure exactly when we consider it safe to break compiling with --no-sound-null-safety.

@domesticmouse

  • Do we still publish a version of "DDC DartPad" that supports weak null safety? Where are the scripts that compile the sound null safe modules for "DDC DartPad" and what .dill files do they use for the SDK?

@annagrin

  • I don't believe the Flutter web builds rely on these .dill files but could you help me confirm? Also what about the debugger?

@jakemac53

  • build_web_compilers will need to update the .dill file paths with this change lands.

DDC SDK .dill Files

Description Current path/file Proposed path/file
Sound null safety outline dart-sdk/lib/_internal/ddc_outline_sound.dill dart-sdk/lib/_internal/ddc_outline.dill
Sound null safety full dart-sdk/lib/_internal/ddc_platform_sound.dill dart-sdk/lib/_internal/ddc_platform.dill
Weak null safety outline dart-sdk/lib/_internal/ddc_sdk.dill DELETED
Weak null safety full dart-sdk/lib/_internal/ddc_platform.dill DELETED

I also want to remove the precompiled SDK JavaScript files. We never shipped the sound null safe versions and instead updated all the integrations of DDC to compile the SDK as needed. We only left the files in the SDK for backwards compatibility which we are breaking with Dart 3.0 anyway. AFAIK we don't use any of the files I want to delete in any supported use case.

@domesticmouse @jakemac53 @annagrin

  • The one file I think we need to keep shipping is the module loading library require.js. AFAIK all of our external uses (webdev, flutter web, DartPad) all rely on a copy of that file. I'm proposing to remove a pointless directory from the path but I'm open to your thoughts on this one.

DDC JavaScript Files

Description Current path/file Proposed path/file
Weak null safety compiled SDK (amd format) dart-sdk/lib/dev_compiler/kernel/amd/dart_sdk.js DELETED
SDK source map dart-sdk/lib/dev_compiler/kernel/amd/dart_sdk.js.map DELETED
Require.js module loader dart-sdk/lib/dev_compiler/kernel/amd/require.js dart-sdk/lib/dev_compiler/amd/require.js
Weak null safety compiled SDK (common format) dart-sdk/lib/dev_compiler/kernel/common/dart_sdk.js DELETED
SDK source map dart-sdk/lib/dev_compiler/kernel/common/dart_sdk.js.map DELETED
Common module loader dart-sdk/lib/dev_compiler/kernel/common/run.js DELETED
Weak null safety compiled SDK (es6 format) dart-sdk/lib/dev_compiler/kernel/es6/dart_sdk.js DELETED
SDK source map dart-sdk/lib/dev_compiler/kernel/es6/dart_sdk.js.map DELETED
@nshahan nshahan added web-dev-compiler area-web Use area-web for Dart web related issues, including the DDC and dart2js compilers and JS interop. labels Dec 13, 2022
@nshahan nshahan self-assigned this Dec 13, 2022
@nshahan nshahan added the P2 A bug or feature request we're likely to work on label Dec 13, 2022
@nshahan
Copy link
Contributor Author

nshahan commented Dec 13, 2022

cc @sigmundch

@domesticmouse
Copy link
Member

There is no null-unsafe version of DartPad at this point. We just run null safe versions of stable, old (stable - 1), beta, and master. (There is a hidden one that has the name dev, which is a clone of stable with different package config)

For the code that Dart Services uses to build DDC artifacts with the dill files, please see https://github.com/dart-lang/dart-services/blob/master/tool/grind.dart#L240

@domesticmouse
Copy link
Member

On the require.js front, please chat with @johnpryan as that is a concern for the DartPad front end, which is something John knows a lot better than I do.

@mit-mit
Copy link
Member

mit-mit commented Dec 13, 2022

I think this very reasonable, and if the only changes are behind the legacy flag, then I do not consider it formally breaking.

@jakemac53
Copy link
Contributor

My only comment would be it might be best to avoid re-using any pre-existing file paths. The errors are likely to be more confusing for the users that are affected, than if we used a totally new path so the old file doesn't exist at all.

@sigmundch
Copy link
Member

/cc @christopherfujino may be able to help answer some of your questions regarding flutter web

@nshahan
Copy link
Contributor Author

nshahan commented Dec 16, 2022

@mit-mit

I think this very reasonable, and if the only changes are behind the legacy flag, then I do not consider it formally breaking.

I just want make sure I know what you mean by "the legacy flag". Do you mean --no-sound-null-safety? I'm essentially removing the files that will support that flag in DDC from the released SDK.

@mit-mit
Copy link
Member

mit-mit commented Dec 19, 2022

I should have said "related to the legacy flag". I have no problem with removing that flag, and the associated support from DDC.

@nshahan
Copy link
Contributor Author

nshahan commented Jan 4, 2023

Update: This work is in progress but we have found tests for package:dwds that expect to run with weak null safety. They need to continue running for now to prevent any regressions. The deletion of the unsound .dill files from the packaged SDK will not happen until after we move those tests to run in a different environment. For now they will be renamed in preparation for the eventual deletion:

Description Current path/file Proposed path/file
Sound null safety outline dart-sdk/lib/_internal/ddc_outline_sound.dill dart-sdk/lib/_internal/ddc_outline.dill
Sound null safety full dart-sdk/lib/_internal/ddc_platform_sound.dill dart-sdk/lib/_internal/ddc_platform.dill
Weak null safety outline dart-sdk/lib/_internal/ddc_sdk.dill dart-sdk/lib/_internal/ddc_outline_unsound.dill
Weak null safety full dart-sdk/lib/_internal/ddc_platform.dill dart-sdk/lib/_internal/ddc_platform_unsound.dill

@domesticmouse
Copy link
Member

For now they will be renamed in preparation for the eventual deletion

When is this scheduled to happen? This change will break dart-lang/dart-services I believe. Given Dart Services supports everything from master to stable - 1 this is going to be an interesting thing to manage through the waterfall.

@nshahan
Copy link
Contributor Author

nshahan commented Jan 5, 2023

I'd like to land this change as soon as possible, but I don't think this will actually break dart-lang/dart-services. Looking at this line it looks like the .dill file being used comes from the Flutter SDK which is built separately.
https://github.com/dart-lang/dart-services/blob/53ace61b517e689fe2bd0d17e2afc24fc58f29d4/lib/src/compiler.dart#L191

I don't see any .dill file being passed that comes from the Dart SDK which is what I am changing.

@nshahan
Copy link
Contributor Author

nshahan commented Jan 5, 2023

@johnpryan Do you know where require.js is being pulled from to load DDC compiled code? That is another file in the Dart SDK that I'm planning on moving in another change.

@nshahan
Copy link
Contributor Author

nshahan commented Jan 6, 2023

Discussed via chat, DartPad has its own copy of require.js checked in so my change should not be a problem.
https://github.com/dart-lang/dart-pad/blob/master/web/scripts/require.js

@annagrin
Copy link
Contributor

annagrin commented Jan 10, 2023

@nshahan Summarizing our discussion offline: we don't want to break webdev tests that are currently running in weak mode, so the incremental plan is in dart-lang/webdev#1878:

  1. dwds tests fixtures: Use special branch of build_web_compilers that supports weak null safety
  2. webdev, dwds test infrastructure: Do not pass --(no)-sound-null-safety flags to build_web_compilers
  3. dwds test infrastructure (using build_web_compilers): Generate js and dill files from sources for weak version of SDK
  4. dwds test infrastructure (using build_web_compilers): Handle old and new names for the SDK assets
  5. SDK: Then we can remove the weak sdk files from the sdk releases.

copybara-service bot pushed a commit that referenced this issue Jan 18, 2023
- Removes all versions of `dart_sdk.js` and `dart_sdk.js.map` from the
  packaged SDK. All supported build systems compile their own versions
  of these files.
- Removes `dart-sdk/lib/dev_compiler/kernel/common/run.js`.
  No supported build system uses the "common" module system.
- Moves `dart-sdk/lib/dev_compiler/kernel/amd/require.js` to
  `dart-sdk/lib/dev_compiler/amd/require.js` cleaning up the
  unnecessary `kernel/` sub-directory.

Issue: #50700

Change-Id: Id4173ddec31a6c0260009924bf1e1ae3d3f32abf
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/275482
Reviewed-by: Mark Zhou <[email protected]>
Commit-Queue: Nicholas Shahan <[email protected]>
jakemac53 added a commit to dart-lang/build that referenced this issue Jan 18, 2023
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.
@mit-mit
Copy link
Member

mit-mit commented Jan 22, 2023

@nshahan I think these changes may have broken our basic webdev workflow? Try these with a recent master (I'm using dart from flutter master):

dart create -t web webapp
cd webapp
dart pub global activate webdev
dart pub global run webdev serve

=>

[SEVERE] build_web_compilers:sdk_js on asset:build_web_compilers/$package$: Error compiling dartdevc module:build_web_compilers|lib/src/dev_compiler/dart_sdk.sound.js

Unhandled exception:
PathNotFoundException: Cannot open file, path = '/Users/mit/dev/flutter/bin/cache/dart-sdk/lib/_internal/ddc_platform_sound.dill' (OS Error: No such file or directory, errno = 2)
mit-macbookpro7:webapp1 mit$ dart pub global run webdev --version
2.7.12

@jakemac53
Copy link
Contributor

@mit-mit you must be on the latest build_runner/build_web_compilers/build_modules for things to work.

@nshahan
Copy link
Contributor Author

nshahan commented Jan 23, 2023

@mit-mit You are right, the dart create -t web webapp command is currently broken. I have an update to the new project template prepared but I've been waiting for all versions of the necessary packages webdev, dwds, build_web_compilers to be published before I updated the template. Those are done or very close but I believe some are blocked on a release of the Dart SDK at or after 3.0.0-134.0.dev which is the first version to include my changes.

@nshahan
Copy link
Contributor Author

nshahan commented Jan 23, 2023

@mit-mit I opened a new issue to track the progress: #51106

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-web Use area-web for Dart web related issues, including the DDC and dart2js compilers and JS interop. P2 A bug or feature request we're likely to work on web-dev-compiler
Projects
None yet
Development

No branches or pull requests

6 participants