Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Support custom entrypoints in public Windows API #35285

Merged
merged 1 commit into from
Aug 10, 2022

Conversation

cbracken
Copy link
Member

@cbracken cbracken commented Aug 9, 2022

This adds a dart_entrypoint field to FlutterDesktopEngineProperties in
the public C Windows API, which mirrors that in the embedder API.

When a null or empty entrypoint is specified, a default entrypoint of
'main' is assumed. Otherwise, the app is launched at the top-level
function specified, which must be annotated with
@pragma('vm:entry-point') in the Dart source.

This change is backward-compatible for existing users of the Windows C API
and the C++ client wrapper API. To avoid breaking backward compatibility,
this patch preserves the entry_point parameter to FlutterDesktopEngineRun
in the public Windows C API as well as in the FlutterEngine::Run method
in the C++ client wrapper API. The entrypoint can be specified in either
the engine properties struct or via the parameter, but if conflicting
non-empty values are specified, the engine launch will intentionally fail
with an error message.

This change has no effect on existing Flutter Windows desktop apps and no
migration is required, because our app templates never specify a custom
entrypoint, nor was the option to specify one via the old method
particularly feasible, because the FlutterViewController class constructor
immediately invokes FlutterViewControllerCreate which
immediately launches the engine passed to it with a null entrypoint
argument, so long as the engine is not already running. However, running the
engine without a view controller previously resulted in errors due to failure to
create a rendering surface.

This is a followup patch to #35273
which added support for running Dart fixture tests with a live Windows
embedder engine.

Fixes: flutter/flutter#93537
Related: flutter/flutter#87299

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
  • I listed at least one issue that this PR fixes in the description above.
  • I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See testing the engine for instructions on
    writing and running engine tests.
  • I updated/added relevant documentation (doc comments with ///).
  • I signed the CLA.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@cbracken
Copy link
Member Author

cbracken commented Aug 9, 2022

I'd be particularly interested in @knopp's review here as the author of an alternative embedder.

There are ways we could do this and fully preserve backward compatibility, though it likely involves coming up with some relatively unfortunate function names (particularly the no-entrypoint variant of FlutterDesktopEngineRun)

Copy link
Member

@loic-sharma loic-sharma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good to me, but I'm conflicted on the breaking change.

Copy link
Member

@loic-sharma loic-sharma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Talked offline, breaking changes shouldn't affect the vast majority of apps. We think the impact is acceptable.

LGTM

@cbracken
Copy link
Member Author

Alright -- did some refactoring to avoid breaking backward-compatibility for windows embedder wrappers. Thanks for pushing back on this; the resulting patch I think gets us to a place where flutter/flutter#93537 is fixed and we have the fixture testing I wanted, plus preserves backward compatibility.

We can deal with the more contentious issue of how we deprecate the entrypoint parameter on FlutterDesktopEngineRun in the C API and FlutterEngine::Run` in the C++ client wrapper separately. I've filed flutter/flutter#109285 to track that.

This adds a dart_entrypoint field to FlutterDesktopEngineProperties in
the public C Windows API, which mirrors that in the embedder API.

When a null or empty entrypoint is specified, a default entrypoint of
'main' is assumed. Otherwise, the app is launched at the top-level
function specified, which must be annotated with
@pragma('vm:entry-point') in the Dart source.

To avoid breaking backward compatibility, this patch preserves the
entry_point parameter to FlutterDesktopEngineRun in the public Windows C
API as well as in the FlutterEngine::Run method. The entrypoint can be
specified in either place, but if conflicting non-empty values are
specified, the engine launch will intentionally fail with an error
message.

Fixes: flutter/flutter#93537
Related: flutter/flutter#87299
//
// If provided, entry_point must be the name of a top-level function from the
// If sprecified, entry_point must be the name of a top-level function from the
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// If sprecified, entry_point must be the name of a top-level function from the
// If specified, entry_point must be the name of a top-level function from the

Copy link
Member

@loic-sharma loic-sharma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-LGTM. Looks great! 🎉

@cbracken cbracken merged commit c3c7e33 into flutter:main Aug 10, 2022
@cbracken cbracken deleted the win-multiple-entrypoints branch August 10, 2022 17:31
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 10, 2022
emilyabest pushed a commit to emilyabest/engine that referenced this pull request Aug 12, 2022
This adds a dart_entrypoint field to FlutterDesktopEngineProperties in
the public C Windows API, which mirrors that in the embedder API.

When a null or empty entrypoint is specified, a default entrypoint of
'main' is assumed. Otherwise, the app is launched at the top-level
function specified, which must be annotated with
@pragma('vm:entry-point') in the Dart source.

This change is backward-compatible for existing users of the Windows C API
and the C++ client wrapper API. To avoid breaking backward compatibility,
this patch preserves the entry_point parameter to FlutterDesktopEngineRun
in the public Windows C API as well as in the FlutterEngine::Run method
in the C++ client wrapper API. The entrypoint can be specified in either
the engine properties struct or via the parameter, but if conflicting
non-empty values are specified, the engine launch will intentionally fail
with an error message.

This change has no effect on existing Flutter Windows desktop apps and no
migration is required, because our app templates never specify a custom
entrypoint, nor was the option to specify one via the old method particularly
feasible, because the FlutterViewController class constructor immediately
invokes FlutterViewControllerCreate which immediately launches the engine
passed to it with a null entrypoint argument, so long as the engine is not
already running. However, running the engine without a view controller
previously resulted in errors due to failure to create a rendering surface.

This is a followup patch to flutter#35273
which added support for running Dart fixture tests with a live Windows
embedder engine.

Fixes: flutter/flutter#93537
Related: flutter/flutter#87299
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for custom entry point for flutter engine on Windows
3 participants