[fuchsia] Create CFv2 dart_runner. #28519
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat. If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
| zx::channel dir; | ||
| if (flat->paths.at(i) == kServiceRootPath) { | ||
| svc = std::make_unique<sys::ServiceDirectory>( | ||
| std::move(flat->directories.at(i))); |
There was a problem hiding this comment.
There is an interesting ordering problem here that I just noticed. If we call PrepareBuiltinLibraries before we call PrepareNamespace we will fail to serve /svc because we use std::move() here whereas in PrepareNamespace we clone this file descriptor. I wonder if it would be safer to clone it in both places or put in some sort of ordering check to make sure this gets called after prepare namespace.
| } | ||
|
|
||
| fdio_ns_t* DartComponentControllerV2::PrepareNamespace() { | ||
| fdio_ns_t* ns; |
There was a problem hiding this comment.
In your above method you set this to nullptr. Do you want to do that here as well to be safe?
| return nullptr; | ||
| } | ||
|
|
||
| if (!start_info_.has_ns()) { |
There was a problem hiding this comment.
nit: this can be moved to the beginning of the method to avoid creating a namespace just to return early.
| } | ||
|
|
||
| // Find the name of the component. | ||
| // fuchsia-pkg://fuchsia.com/hello_dart#meta/hello_dart.cm -> hello_dart |
There was a problem hiding this comment.
One note: this is something I added in my original PR since it matches how we build flutter applications in tree right now. In the future we will probably want to avoid making this assumption and only use it as a default. We should define an API for the cml file which allows users to set this value directly and then pull that out from the program dictionary in the startup info https://cs.opensource.google/fuchsia/fuchsia/+/main:sdk/fidl/fuchsia.component.runner/component_runner.fidl;l=64.
|
Spoke with David offline yesterday, going to take back this PR in favor of a different approach that's less invasive/risky given the previous breakage (going to avoid the base class). |
2 participants
Bug: 79871
Creates a common interface for a DartComponentController and moves the current component controller into a V1 implementation, creating a V2 implementation for the new version.
The code is still using the CFv1 implementation, so this should not affect the existing behavior if it is working properly.
Relands GN and C++ changes for #27226 after rollback in #28036. I believe the flake was being caused by namespace_ not being reset to nullptr after calling fdio_ns_create(), leading to the namespace being corrupted during a second+ run of the component controller. But this still needs to be tested and verified.