Skip to content

Support the --{no-}sound-null-safety flags in the kernel worker #43091

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
jakemac53 opened this issue Aug 18, 2020 · 4 comments
Closed

Support the --{no-}sound-null-safety flags in the kernel worker #43091

jakemac53 opened this issue Aug 18, 2020 · 4 comments
Assignees
Labels
customer-bazel legacy-area-front-end Legacy: Use area-dart-model instead. NNBD Issues related to NNBD Release

Comments

@jakemac53
Copy link
Contributor

The kernel worker (used by build_runner and google3) does not currently plumb through or support the --{no-}sound-null-safety flag, this will be a requirement for running in sound null safety mode with our modular builds.

There are some open questions as to exactly how it should be implemented:

  • Should we throw away all cached modules if the flag changes from one invocation to the next?
    • If we do this then probably blaze and build_runner would run twice as many workers, keeping a set for each mode and delegating requests to the respective workers.
  • Or should we keep two incremental compilers around, and two caches of components, so that we can more efficiently switch back and forth?

I believe DDC already supports this flag, but I don't know how it is handling invalidation issue listed above, cc @nshahan @Markzipan

cc @jensjoha @sigmundch @natebosch @davidmorgan

@jakemac53 jakemac53 added customer-bazel NNBD Issues related to NNBD Release legacy-area-front-end Legacy: Use area-dart-model instead. labels Aug 18, 2020
@jakemac53
Copy link
Contributor Author

assigned to the area-front-end for right now for a lack of a more obvious area to assign it to

@nshahan
Copy link
Contributor

nshahan commented Aug 18, 2020

I believe DDC already supports this flag, but I don't know how it is handling invalidation issue listed above

I haven't tested it properly in the worker configuration but I added checks to invalidate if the mode changes and to error if you try to compile with --sound-null-safety and use weak summary .dills for the sdk or dependencies.
https://dart-review.googlesource.com/c/sdk/+/146962/17/pkg/front_end/lib/src/api_unstable/modular_incremental_compilation.dart

I might have missed something though if we find the invalidation isn't enough.

@jakemac53
Copy link
Contributor Author

Nice @nshahan that is a fine option I think - and that is shared code so we just need to plumb the flag through then. I can work on that.

@jakemac53 jakemac53 self-assigned this Aug 18, 2020
dart-bot pushed a commit that referenced this issue Aug 19, 2020
Bug:#43091
Change-Id: I716d2ea6cf1c95be3911c32b1b2958788fe917ef
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/159183
Reviewed-by: Jens Johansen <[email protected]>
Reviewed-by: Nicholas Shahan <[email protected]>
Commit-Queue: Jake Macdonald <[email protected]>
@jakemac53
Copy link
Contributor Author

This was resolved in 1522a86

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
customer-bazel legacy-area-front-end Legacy: Use area-dart-model instead. NNBD Issues related to NNBD Release
Projects
None yet
Development

No branches or pull requests

2 participants