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

Conversation

@stuartmorgan-g
Copy link
Contributor

The embedder.h API layer is an implementation detail of the desktop
embeddings, not part of the public API surface, so should not be part of
the public symbol list for those libraries.

The embedder.h API layer is an implementation detail of the desktop
embeddings, not part of the public API surface, so should not be part of
the public symbol list for those libraries.
@auto-assign auto-assign bot requested a review from iskakaushik June 2, 2020 13:07
@stuartmorgan-g
Copy link
Contributor Author

I meant to fix this a long time ago, then forgot until someone happened to post a list of public symbols in flutter_windows.dll while asking about something else.

Copy link
Member

@chinmaygarde chinmaygarde left a comment

Choose a reason for hiding this comment

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

Minor nit about the name of the define. But fine either way.

@stuartmorgan-g
Copy link
Contributor Author

Re-runs were both green. Will land once the tree is open.

@stuartmorgan-g
Copy link
Contributor Author

luci-engine redness is flake that's been happening somewhat frequently on that bot; landing.

@stuartmorgan-g stuartmorgan-g merged commit a95882b into flutter:master Jun 4, 2020
@stuartmorgan-g stuartmorgan-g deleted the fix-desktop-embedder-symbol-visibility branch June 4, 2020 01:14
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 4, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 4, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 4, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 4, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 4, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 5, 2020
@ohir
Copy link

ohir commented Jun 6, 2020

@stuartmorgan, @chinmaygarde

This PR actually trashed a few months of work of go-flutter-desktop team. Please advise them about possible workarounds.

Will defining FLUTTER_NO_EXPORT be enough?

@stuartmorgan-g
Copy link
Contributor Author

stuartmorgan-g commented Jun 6, 2020

This PR actually trashed a few months of work of go-flutter-desktop team.

The pull request you linked to is less than a week old, so I don't see where you are getting the "a few months of work".

The desktop embeddings were never intended to export the embedder.h API, and the fact that they have been up until now was a bug. What was being done in that PR was relying on that bug; it was never something we supported. It also, IMO, was not a good idea even despite that. It added completely unnecessary extra runtime dependencies on every client of their project (e.g., forcing all users of go-flutter-desktop applications on Linux to have GTK installed even though their project doesn't use GTK), and increased binary size.

Please advise them about possible workarounds.

I disagree strongly with your use of the term "workaround" here. The correct way for a custom embedder to implement release mode is to build and use a release build of the flutter_engine library (the one that project has always used). Based on both the discussion there, and the bug referenced in the PR you linked to, they are already aware of that, so I don't know what advice you want me to give them. If they have questions, I'm happy to answer them.

Will defining FLUTTER_NO_EXPORT be enough?

Flutter's build system already supports building exactly the artifact they need (which, again, I believe they are already aware of since they link to the page where the flutter-rs project is doing exactly that) with no special configuration. Doing a custom build of the official desktop embedding libraries with FLUTTER_NO_EXPORT defined would make no sense, as it would be more work than building the correct artifact, and give worse outcomes (per my comment above).

I'm sorry that my forgetting to fix this last year led to some wasted effort. I think you're dramatically overestimating how much though, as a fair amount of what's in the PR you linked to appears to be about supporting release mode in general, which will apply just as much to using a release build of the correct library.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants