Skip to content

[linux/window] Fix exported API surface to be DLL/so-safe #230

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
CallumIddon opened this issue Jan 12, 2019 · 7 comments · Fixed by #272
Closed

[linux/window] Fix exported API surface to be DLL/so-safe #230

CallumIddon opened this issue Jan 12, 2019 · 7 comments · Fixed by #272
Assignees
Labels
bug Something isn't working

Comments

@CallumIddon
Copy link
Contributor

VS builds now have a large number of export warnings since #216 landed.

Log:

"C:\dev\flutter-desktop-embedding\example\windows\Example Embedder.sln" (default target) (1) ->
"C:\dev\flutter-desktop-embedding\example\windows\GLFW Example.vcxproj.metaproj" (default target) (2) ->
"C:\dev\flutter-desktop-embedding\library\windows\GLFW Library.vcxproj" (default target) (3) ->
(ClCompile target) ->
  c:\dev\flutter-desktop-embedding\library\include\flutter_desktop_embedding\method_call.h(46): warning C4251: 'flutter_desktop_embedding::MethodCal
l<T>::method_name_': class 'std::basic_string<char,std::char_traits<char>,std::allocator<char>>' needs to have dll-interface to be used by clients o
f class 'flutter_desktop_embedding::MethodCall<T>' [C:\dev\flutter-desktop-embedding\library\windows\GLFW Library.vcxproj]
  c:\dev\flutter-desktop-embedding\library\include\flutter_desktop_embedding\method_call.h(47): warning C4251: 'flutter_desktop_embedding::MethodCal
l<T>::arguments_': class 'std::unique_ptr<_Ty,std::default_delete<_Ty>>' needs to have dll-interface to be used by clients of class 'flutter_desktop
_embedding::MethodCall<T>' [C:\dev\flutter-desktop-embedding\library\windows\GLFW Library.vcxproj]
  c:\dev\flutter-desktop-embedding\library\include\flutter_desktop_embedding\engine_method_result.h(47): warning C4251: 'flutter_desktop_embedding::
internal::ReplyManager::reply_handler_': class 'std::function<void (const uint8_t *,size_t)>' needs to have dll-interface to be used by clients of c
lass 'flutter_desktop_embedding::internal::ReplyManager' [C:\dev\flutter-desktop-embedding\library\windows\GLFW Library.vcxproj]
  c:\dev\flutter-desktop-embedding\library\include\flutter_desktop_embedding\engine_method_result.h(84): warning C4251: 'flutter_desktop_embedding::
EngineMethodResult<T>::reply_manager_': class 'std::unique_ptr<flutter_desktop_embedding::internal::ReplyManager,std::default_delete<_Ty>>' needs to
 have dll-interface to be used by clients of class 'flutter_desktop_embedding::EngineMethodResult<T>' [C:\dev\flutter-desktop-embedding\library\wind
ows\GLFW Library.vcxproj]
  c:\dev\flutter-desktop-embedding\library\include\flutter_desktop_embedding\basic_message_channel.h(100): warning C4251: 'flutter_desktop_embedding
::BasicMessageChannel<T>::name_': class 'std::basic_string<char,std::char_traits<char>,std::allocator<char>>' needs to have dll-interface to be used
 by clients of class 'flutter_desktop_embedding::BasicMessageChannel<T>' [C:\dev\flutter-desktop-embedding\library\windows\GLFW Library.vcxproj]
  c:\dev\flutter-desktop-embedding\library\include\flutter_desktop_embedding\method_call.h(46): warning C4251: 'flutter_desktop_embedding::MethodCal
l<T>::method_name_': class 'std::basic_string<char,std::char_traits<char>,std::allocator<char>>' needs to have dll-interface to be used by clients o
f class 'flutter_desktop_embedding::MethodCall<T>' [C:\dev\flutter-desktop-embedding\library\windows\GLFW Library.vcxproj]
  c:\dev\flutter-desktop-embedding\library\include\flutter_desktop_embedding\method_call.h(47): warning C4251: 'flutter_desktop_embedding::MethodCal
l<T>::arguments_': class 'std::unique_ptr<_Ty,std::default_delete<_Ty>>' needs to have dll-interface to be used by clients of class 'flutter_desktop
_embedding::MethodCall<T>' [C:\dev\flutter-desktop-embedding\library\windows\GLFW Library.vcxproj]
  c:\dev\flutter-desktop-embedding\library\include\flutter_desktop_embedding\engine_method_result.h(47): warning C4251: 'flutter_desktop_embedding::
internal::ReplyManager::reply_handler_': class 'std::function<void (const uint8_t *,size_t)>' needs to have dll-interface to be used by clients of c
lass 'flutter_desktop_embedding::internal::ReplyManager' [C:\dev\flutter-desktop-embedding\library\windows\GLFW Library.vcxproj]
  c:\dev\flutter-desktop-embedding\library\include\flutter_desktop_embedding\engine_method_result.h(84): warning C4251: 'flutter_desktop_embedding::
EngineMethodResult<T>::reply_manager_': class 'std::unique_ptr<flutter_desktop_embedding::internal::ReplyManager,std::default_delete<_Ty>>' needs to
 have dll-interface to be used by clients of class 'flutter_desktop_embedding::EngineMethodResult<T>' [C:\dev\flutter-desktop-embedding\library\wind
ows\GLFW Library.vcxproj]
  c:\dev\flutter-desktop-embedding\library\include\flutter_desktop_embedding\method_channel.h(93): warning C4251: 'flutter_desktop_embedding::Method
Channel<T>::name_': class 'std::basic_string<char,std::char_traits<char>,std::allocator<char>>' needs to have dll-interface to be used by clients of
 class 'flutter_desktop_embedding::MethodChannel<T>' [C:\dev\flutter-desktop-embedding\library\windows\GLFW Library.vcxproj]
  c:\dev\flutter-desktop-embedding\library\common\glfw\embedder.cc(229): warning C4305: 'argument': truncation from 'double' to 'GLclampf' [C:\dev\f
lutter-desktop-embedding\library\windows\GLFW Library.vcxproj]
  c:\dev\flutter-desktop-embedding\library\common\glfw\embedder.cc(272): warning C4267: '=': conversion from 'size_t' to 'int', possible loss of dat
a [C:\dev\flutter-desktop-embedding\library\windows\GLFW Library.vcxproj]
  c:\dev\flutter-desktop-embedding\library\common\glfw\embedder.cc(319): warning C4267: 'argument': conversion from 'size_t' to 'int', possible loss
 of data [C:\dev\flutter-desktop-embedding\library\windows\GLFW Library.vcxproj]
  c:\dev\flutter-desktop-embedding\library\include\flutter_desktop_embedding\basic_message_channel.h(100): warning C4251: 'flutter_desktop_embedding
::BasicMessageChannel<T>::name_': class 'std::basic_string<char,std::char_traits<char>,std::allocator<char>>' needs to have dll-interface to be used
 by clients of class 'flutter_desktop_embedding::BasicMessageChannel<T>' [C:\dev\flutter-desktop-embedding\library\windows\GLFW Library.vcxproj]
  c:\dev\flutter-desktop-embedding\library\include\flutter_desktop_embedding\basic_message_channel.h(100): warning C4251: 'flutter_desktop_embedding
::BasicMessageChannel<Json::Value>::name_': class 'std::basic_string<char,std::char_traits<char>,std::allocator<char>>' needs to have dll-interface
to be used by clients of class 'flutter_desktop_embedding::BasicMessageChannel<Json::Value>' [C:\dev\flutter-desktop-embedding\library\windows\GLFW
Library.vcxproj]
  c:\dev\flutter-desktop-embedding\library\include\flutter_desktop_embedding\method_call.h(46): warning C4251: 'flutter_desktop_embedding::MethodCal
l<T>::method_name_': class 'std::basic_string<char,std::char_traits<char>,std::allocator<char>>' needs to have dll-interface to be used by clients o
f class 'flutter_desktop_embedding::MethodCall<T>' [C:\dev\flutter-desktop-embedding\library\windows\GLFW Library.vcxproj]
  c:\dev\flutter-desktop-embedding\library\include\flutter_desktop_embedding\method_call.h(47): warning C4251: 'flutter_desktop_embedding::MethodCal
l<T>::arguments_': class 'std::unique_ptr<_Ty,std::default_delete<_Ty>>' needs to have dll-interface to be used by clients of class 'flutter_desktop
_embedding::MethodCall<T>' [C:\dev\flutter-desktop-embedding\library\windows\GLFW Library.vcxproj]
  c:\dev\flutter-desktop-embedding\library\include\flutter_desktop_embedding\engine_method_result.h(47): warning C4251: 'flutter_desktop_embedding::
internal::ReplyManager::reply_handler_': class 'std::function<void (const uint8_t *,size_t)>' needs to have dll-interface to be used by clients of c
lass 'flutter_desktop_embedding::internal::ReplyManager' [C:\dev\flutter-desktop-embedding\library\windows\GLFW Library.vcxproj]
  c:\dev\flutter-desktop-embedding\library\include\flutter_desktop_embedding\engine_method_result.h(84): warning C4251: 'flutter_desktop_embedding::
EngineMethodResult<T>::reply_manager_': class 'std::unique_ptr<flutter_desktop_embedding::internal::ReplyManager,std::default_delete<_Ty>>' needs to
 have dll-interface to be used by clients of class 'flutter_desktop_embedding::EngineMethodResult<T>' [C:\dev\flutter-desktop-embedding\library\wind
ows\GLFW Library.vcxproj]
  c:\dev\flutter-desktop-embedding\library\include\flutter_desktop_embedding\method_channel.h(93): warning C4251: 'flutter_desktop_embedding::Method
Channel<T>::name_': class 'std::basic_string<char,std::char_traits<char>,std::allocator<char>>' needs to have dll-interface to be used by clients of
 class 'flutter_desktop_embedding::MethodChannel<T>' [C:\dev\flutter-desktop-embedding\library\windows\GLFW Library.vcxproj]
  c:\dev\flutter-desktop-embedding\library\include\flutter_desktop_embedding\method_channel.h(93): warning C4251: 'flutter_desktop_embedding::Method
Channel<Json::Value>::name_': class 'std::basic_string<char,std::char_traits<char>,std::allocator<char>>' needs to have dll-interface to be used by
clients of class 'flutter_desktop_embedding::MethodChannel<Json::Value>' [C:\dev\flutter-desktop-embedding\library\windows\GLFW Library.vcxproj]
  c:\dev\flutter-desktop-embedding\library\include\flutter_desktop_embedding\method_call.h(46): warning C4251: 'flutter_desktop_embedding::MethodCal
l<Json::Value>::method_name_': class 'std::basic_string<char,std::char_traits<char>,std::allocator<char>>' needs to have dll-interface to be used by
 clients of class 'flutter_desktop_embedding::MethodCall<Json::Value>' [C:\dev\flutter-desktop-embedding\library\windows\GLFW Library.vcxproj]
  c:\dev\flutter-desktop-embedding\library\include\flutter_desktop_embedding\method_call.h(47): warning C4251: 'flutter_desktop_embedding::MethodCal
l<Json::Value>::arguments_': class 'std::unique_ptr<T,std::default_delete<_Ty>>' needs to have dll-interface to be used by clients of class 'flutter
_desktop_embedding::MethodCall<Json::Value>' [C:\dev\flutter-desktop-embedding\library\windows\GLFW Library.vcxproj]
  c:\dev\flutter-desktop-embedding\library\include\flutter_desktop_embedding\engine_method_result.h(84): warning C4251: 'flutter_desktop_embedding::
EngineMethodResult<T>::reply_manager_': class 'std::unique_ptr<flutter_desktop_embedding::internal::ReplyManager,std::default_delete<_Ty>>' needs to
 have dll-interface to be used by clients of class 'flutter_desktop_embedding::EngineMethodResult<T>' [C:\dev\flutter-desktop-embedding\library\wind
ows\GLFW Library.vcxproj]
  c:\dev\flutter-desktop-embedding\library\include\flutter_desktop_embedding\method_call.h(46): warning C4251: 'flutter_desktop_embedding::MethodCal
l<T>::method_name_': class 'std::basic_string<char,std::char_traits<char>,std::allocator<char>>' needs to have dll-interface to be used by clients o
f class 'flutter_desktop_embedding::MethodCall<T>' [C:\dev\flutter-desktop-embedding\library\windows\GLFW Library.vcxproj]
  c:\dev\flutter-desktop-embedding\library\include\flutter_desktop_embedding\method_call.h(47): warning C4251: 'flutter_desktop_embedding::MethodCal
l<T>::arguments_': class 'std::unique_ptr<_Ty,std::default_delete<_Ty>>' needs to have dll-interface to be used by clients of class 'flutter_desktop
_embedding::MethodCall<T>' [C:\dev\flutter-desktop-embedding\library\windows\GLFW Library.vcxproj]
  c:\dev\flutter-desktop-embedding\library\include\flutter_desktop_embedding\engine_method_result.h(47): warning C4251: 'flutter_desktop_embedding::
internal::ReplyManager::reply_handler_': class 'std::function<void (const uint8_t *,size_t)>' needs to have dll-interface to be used by clients of c
lass 'flutter_desktop_embedding::internal::ReplyManager' [C:\dev\flutter-desktop-embedding\library\windows\GLFW Library.vcxproj]
  c:\dev\flutter-desktop-embedding\library\include\flutter_desktop_embedding\engine_method_result.h(84): warning C4251: 'flutter_desktop_embedding::
EngineMethodResult<T>::reply_manager_': class 'std::unique_ptr<flutter_desktop_embedding::internal::ReplyManager,std::default_delete<_Ty>>' needs to
 have dll-interface to be used by clients of class 'flutter_desktop_embedding::EngineMethodResult<T>' [C:\dev\flutter-desktop-embedding\library\wind
ows\GLFW Library.vcxproj]
  c:\dev\flutter-desktop-embedding\library\include\flutter_desktop_embedding\method_channel.h(93): warning C4251: 'flutter_desktop_embedding::Method
Channel<T>::name_': class 'std::basic_string<char,std::char_traits<char>,std::allocator<char>>' needs to have dll-interface to be used by clients of
 class 'flutter_desktop_embedding::MethodChannel<T>' [C:\dev\flutter-desktop-embedding\library\windows\GLFW Library.vcxproj]
  c:\dev\flutter-desktop-embedding\library\include\flutter_desktop_embedding\method_call.h(46): warning C4251: 'flutter_desktop_embedding::MethodCal
l<T>::method_name_': class 'std::basic_string<char,std::char_traits<char>,std::allocator<char>>' needs to have dll-interface to be used by clients o
f class 'flutter_desktop_embedding::MethodCall<T>' [C:\dev\flutter-desktop-embedding\library\windows\GLFW Library.vcxproj]
  c:\dev\flutter-desktop-embedding\library\include\flutter_desktop_embedding\method_call.h(47): warning C4251: 'flutter_desktop_embedding::MethodCal
l<T>::arguments_': class 'std::unique_ptr<_Ty,std::default_delete<_Ty>>' needs to have dll-interface to be used by clients of class 'flutter_desktop
_embedding::MethodCall<T>' [C:\dev\flutter-desktop-embedding\library\windows\GLFW Library.vcxproj]
  c:\dev\flutter-desktop-embedding\library\include\flutter_desktop_embedding\method_call.h(46): warning C4251: 'flutter_desktop_embedding::MethodCal
l<T>::method_name_': class 'std::basic_string<char,std::char_traits<char>,std::allocator<char>>' needs to have dll-interface to be used by clients o
f class 'flutter_desktop_embedding::MethodCall<T>' [C:\dev\flutter-desktop-embedding\library\windows\GLFW Library.vcxproj]
  c:\dev\flutter-desktop-embedding\library\include\flutter_desktop_embedding\method_call.h(47): warning C4251: 'flutter_desktop_embedding::MethodCal
l<T>::arguments_': class 'std::unique_ptr<T,std::default_delete<_Ty>>' needs to have dll-interface to be used by clients of class 'flutter_desktop_e
mbedding::MethodCall<T>' [C:\dev\flutter-desktop-embedding\library\windows\GLFW Library.vcxproj]
@stuartmorgan-g
Copy link
Collaborator

Not having a Windows development background, I was not aware of the severe limitations on what can safely be used in a DLL interface. (Having seen this export pattern in Chromium, I didn't expect issues, but Chromium's build doesn't create DLLs for distribution).

It look like the Windows API surface will need to be completely rethought.

@stuartmorgan-g
Copy link
Collaborator

Although since most of the current approach is templated, and thus entirely in headers, it may be salvageable with some adjustments. I'll need to do more research.

@CallumIddon
Copy link
Contributor Author

I have gotten rid of the warnings by removing the export from the class and adding it to each method and member individually but leaving out the private members (IIRC shouldn't need exporting). I still want to test this further before creating a PR with some simple plugins and embedders to see if this causes any issues. Linux and macOS still pass testing so I think everything will be okay.

@stuartmorgan-g
Copy link
Collaborator

The issue isn't so much the warnings as the problem of the STL not having a stable ABI, making the use of the STL in DLL interfaces untenable. (Which I became aware of while reading up on the warning.)

Things work right now because everything is being compiled at once by the same compiler, with the same settings. That's not something that can be guaranteed in a binary distribution model.

@stuartmorgan-g stuartmorgan-g self-assigned this Jan 14, 2019
@stuartmorgan-g stuartmorgan-g added the bug Something isn't working label Jan 14, 2019
@stuartmorgan-g stuartmorgan-g changed the title [Windows] Export Warnings [linux/window] Fix exported API surface to be DLL/so-safe Jan 14, 2019
@stuartmorgan-g
Copy link
Collaborator

Retitling for the expanded scope. Currently my thinking is that I'll pare the actual library interface down to a small number of abstract interfaces with no STL use. The bulk of the code will remain template headers, which won't actually be exported from the DLL, just available for clients to use, so that in practice the client code API surface will look much the same, even though the DLL API is pared down. I'll likely need to add one or two utility headers to wrap the raw memory management that the API will need to use with smart pointers, so that client code doesn't have that foot-gun to deal with.

I haven't implemented this yet, so it may end up being more extensive than I'm currently hoping, but given how much is currently pure-header I'm hopeful that it won't be a huge change.

@clarkezone
Copy link
Collaborator

clarkezone commented Jan 15, 2019 via email

@stuartmorgan-g
Copy link
Collaborator

From a cursory look at how APIs work it's not clear to me what the compat implications are; potentially? If it's using something with strong binary compat as the actual boundary, and then transparently layering C++ over that for the caller, that could certainly be useful for the Windows-specific APIs (that's what I'm hoping do to more or less do with the plugin APIs, only manually).

stuartmorgan-g added a commit to stuartmorgan-g/flutter-desktop-embedding that referenced this issue Jan 28, 2019
Creates an initial C++ object as the primary intaction point for the
embedder calls. This provides a simpler API surface than embedder.h,
and folds in some common code (e.g., error logging).

This also serves to insulate clients from the embedder.h API layer,
so that future incremental changes done for google#230 will cause less churn
for embedders.
stuartmorgan-g pushed a commit that referenced this issue Jan 29, 2019
Creates a simple C++ object as the primary interaction point for the
embedder calls. This provides a simpler API surface than embedder.h,
and folds in some common code (e.g., error logging).

This also serves to insulate clients from the embedder.h API layer,
so that future incremental changes done for #230 will cause less churn
for embedders.
stuartmorgan-g added a commit to stuartmorgan-g/flutter-desktop-embedding that referenced this issue Jan 29, 2019
Rather than exposing the GLFWwindow that is used, treat it as an
implementation detail and use an opaque pointer specific to this library
as the context object instead.

In practice the use of GLFW can't be entirely internal since the use of
GLFW in the library prevents its use by the embedder, but the details of
how it is used should not be exposed. This also gives greater control
over the API surface, as part of working toward a stable ABI (google#230).

This does mean that the embedder cannot manipulate the GLFW window
directly, but rather than trust that doing so would be safe across DLL
boundaries, any functionality need by the simple embedder
use cases this library enables should be exposed via wrappers.
stuartmorgan-g pushed a commit that referenced this issue Jan 29, 2019
Rather than exposing the GLFWwindow that is used, treat it as an
implementation detail and use an opaque pointer specific to this library
as the context object instead.

In practice the use of GLFW can't be entirely internal since the use of
GLFW in the library prevents its use by the embedder, but the details of
how it is used should not be exposed. This also gives greater control
over the API surface, as part of working toward a stable ABI (#230).

This does mean that the embedder cannot manipulate the GLFW window
directly, but rather than trust that doing so would be safe across DLL
boundaries, any functionality need by the simple embedder
use cases this library enables should be exposed via wrappers.
stuartmorgan-g added a commit to stuartmorgan-g/flutter-desktop-embedding that referenced this issue Jan 31, 2019
Eliminates PluginRegistrar and BinaryMessenger from the embedder.h API
surface, moving them up to the FlutterWindowController level. In their
place, adds new low-level messaging APIs, which are based closely on
the Flutter engine embedder APIs (including an equivalent message
struct), in some cases wrapping them directly. Unlike the engine APIs,
they allow per-channel callbacks, so individual plugins can use the API.

Since PluginHandler is no longer the source of truth for callback
registration, only an adaptor, it's now fine to have multiple instances
of them, all passing through to the underlying C API.

Part of google#230
stuartmorgan-g pushed a commit that referenced this issue Jan 31, 2019
Eliminates PluginRegistrar and BinaryMessenger from the embedder.h API
surface, moving them up to the FlutterWindowController level. In their
place, adds new low-level messaging APIs, which are based closely on
the Flutter engine embedder APIs (including an equivalent message
struct), in some cases wrapping them directly. Unlike the engine APIs,
they allow per-channel callbacks, so individual plugins can use the API.

Since PluginHandler is no longer the source of truth for callback
registration, only an adaptor, it's now fine to have multiple instances
of them, all passing through to the underlying C API.

Part of #230
stuartmorgan-g added a commit to stuartmorgan-g/flutter-desktop-embedding that referenced this issue Feb 4, 2019
Remove remaining C++ from embedder.h, and add extern C.

Part of google#230
stuartmorgan-g pushed a commit that referenced this issue Feb 4, 2019
Remove remaining C++ from embedder.h, and add extern C. Since the
namespace is removed, use a standard prefix for all function names.

Part of #230
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 10, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants