Skip to content
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

Enable extras/module-node and examples/debug-trans-socket to be used in DLLs #2020

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

ghost
Copy link

@ghost ghost commented Dec 30, 2018

When deploying Duktape as a DLL it is sometimes useful to bundle some extra functionality, compared to what vanilla duktape.h provides. In my case I wanted to be able to deploy duktape.dll with support for Node.js modules and debugging out of the box . However, it seems that these extra modules are not ready to be used that way because of missing __declspec specifiers.

I understand I could put these modules in separate DLLs (like duktape-module-node.dll etc), but it seems a bit overkill, and even then I would need to resolve __declspec specifiers, which would require me to modify sources that come with Duktape, which isn't perfect from maintenance perspective.

@ghost
Copy link
Author

ghost commented Jun 2, 2019

Bump. Something wrong with this PR? Since Duktape is DLL-ready then it seems reasonable to make at least Node-style modules ready too.

@svaarala
Copy link
Owner

svaarala commented Jun 2, 2019

The main thing is that those visibility macros are not intended for external use.

@svaarala
Copy link
Owner

svaarala commented Jun 2, 2019

Also setting DUK_COMPILING_DUKTAPE for non-duktape.c sources is similarly not intended - it is only intended for compiling Duktape itself and forcing it here may be fragile.

@svaarala
Copy link
Owner

svaarala commented Jun 2, 2019

One approach would be to intentionally expose the visibility symbols as part of the documented API so that calling code could rely on them with versioning guarantees. That would eliminate the need to force DUK_COMPILING_DUKTAPE as the macros would be visible even without it. The macros have been through a few iterations so I guess they may be stable enough to make part of the versioning guarantees.

@ghost
Copy link
Author

ghost commented Jun 2, 2019

I guess different export/import macros could be used for non-duktape.c sources. As long as required symbols can be exported from a DLL then it should be fine.

@svaarala
Copy link
Owner

svaarala commented Jun 2, 2019

Looking at the definition of DUK_EXTERNAL_DECL the main issue when compiling on Windows is that the necessary __declspec() is different depending on whether you're compiling the library (export) or the calling code (import).

DUK_EXTERNAL_DECL is actually available for public use already as the API symbols use it. DUK_COMPILING_DUKTAPE controls whether we're compiling the library or the call site, and which variant of __declspec() is used (export or import).

I'm not sure what would be a clean solution. Suppose for example that a file wanted to simultaneously call Duktape API (import symbols) and declare externally visible DLL symbols (export symbols); no single DUK_EXTERNAL_DECL would work in this case? There would maybe be a need to have separate explicit macros to declare a symbol to be exported or imported (which is now implicitly controlled via DUK_COMPILING_DUKTAPE and which is limited to Duktape only).

@svaarala
Copy link
Owner

svaarala commented Jun 2, 2019

Also, any library that wants to use a certain header file both when compiling the library itself (export) and the calling code (import) will need to distinguish the compilation context using something similar to DUK_COMPILING_DUKTAPE used by Duktape itself.

So maybe Duktape headers could provide explicit DUK_EXPORT_DECL and DUK_IMPORT_DECL macros, which would map to the __declspec() attrributes on Windows, and the library itself would then use something like this in its headers:

#if defined(COMPILING_TRANS_SOCKET)
#define MY_API_SYMBOL DUK_EXPORT_DECL
#else
#define MY_API_SYMBOL DUK_IMPORT_DECL
#endif

MY_API_SYMBOL void foo(void);
MY_API_SYMBOL void bar(void);

This is pretty much what Duktape does itself in duk_config.h, though there are no intermediate defines like DUK_EXPORT_DECL and DUK_IMPORT_DECL.

Not sure if this is clean enough though?

@ghost
Copy link
Author

ghost commented Jun 2, 2019

Yes, in the case you described separate macros would be needed. The solution you proposed seems to be the standard practice and the way to go.

One more thing worth considering is users that already might be using these extra features (e.g Node modules) by directly jamming them in their custom build targets (i.e. linking against pre-built duktape.dll, but building duk_module_node.c directly in their projects). This, however, shouldn't really be a problem - you can have a __declspec(dllexport) function in an EXE, and still be able to call it. The difference is that it actually gets added to the export table of an EXE file, but in most cases this shouldn't really matter.

When I made my changes I figured that these extra features could be treated as optional, but standard extensions to Duktape's core functionality. That's why I re-used existing macros. But your concerns are valid if you want to keep separation between the core and extensions. Separate macros would definitely be a cleaner approach.

@svaarala
Copy link
Owner

svaarala commented Jun 2, 2019

That's why I re-used existing macros. But your concerns are valid if you want to keep separation between the core and extensions. Separate macros would definitely be a cleaner approach.

That's definitely a goal, the examples/extras could be moved to separate repos and should function as normal calling code from Duktape perspective.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant