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

fix: compilation with BUILD_SHARED_LIBS with MSVC #776

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

Conversation

Nerixyz
Copy link
Contributor

@Nerixyz Nerixyz commented Dec 28, 2024

This fixes the compilation when building shared libraries instead of a statically linked executable. Building a shared library is required for #581. Since MSVC requires __declspec(dllexport), there were some linker errors. Additionally, there were some compiler errors about implicit special member functions being generated in include/dom/ (using clang-cl/lld-link as the compiler leads to much more helpful errors than cl/link).

For GCC-ish compilers, -fvisibility=hidden doesn't seem to be specified for mrdocs-core, because it's a static library. If it was specified, MRDOCS_DECL for GCC would need to be updated (similar to what CMake's GenerateExportHeader does).

Aside: A clang-format config would be great.

Footnotes

  1. Technically, the executable could still be statically linked while exporting the required symbols. That's basically what setting MRDOCS_BUILD_SHARED=ON without enabling BUILD_SHARED_LIBS does. I think ENABLE_EXPORTS would still be required for that executable to make sure an "import library" is created.

@cppalliance-bot
Copy link

An automated preview of the documentation is available at https://776.mrdocs.prtest2.cppalliance.org/index.html

@cppalliance-bot
Copy link

An automated preview of the documentation is available at https://776.mrdocs.prtest2.cppalliance.org/index.html

@alandefreitas
Copy link
Collaborator

That's amazing, especially since it's now being checked in CI. The plugin system is not designed yet but it's a step we need to take. I'll review it in 2025.

@alandefreitas
Copy link
Collaborator

Just had a look at this. The changes are great. These MRDOCS_DECLs were being tested at all. Just one question though: isn't the requirement #58 the other way around? That is, the compiled plugin is what needs to be a shared library but it doesn't really matter if mrdocs it's static. MrDocs only needs to load it when the system is implemented.

@Nerixyz
Copy link
Contributor Author

Nerixyz commented Jan 2, 2025

Yeah, that's what I initially thought, too. I was a bit confused by the BUILD_SHARED_LIBS option (this should only be honored for plugins, not for the core library). I think we shouldn't build a shared lib for mrdocs (core) at all, only for the plugins:

graph TD;
    implib["mrdocs.lib<br>(import lib)"]
    lib["mrdocs-core.lib<br>(static library with “public“ symbols)"]
    lib---|linked to|mrdocs.exe;
    mrdocs.exe---|derived|implib;
    implib---|“linked“ to|plugin.dll;
    mrdocs.exe---|loads|plugin.dll;
Loading

I'm not entirely sure what the equivalent of an import library on Linux and macOS is.

@alandefreitas
Copy link
Collaborator

Yeah, that's what I initially thought, too. I was a bit confused by the BUILD_SHARED_LIBS option (this should only be honored for plugins, not for the core library).

Yes. If the user asks for BUILD_SHARED_LIBS when invoking CMake, we want to honor that. So that's what MRDOCS_DECL does.

I think we shouldn't build a shared lib for mrdocs (core) at all, only for the plugins:

Now, I'm not really sure about this. I think we can only find out once the plugin system is really implemented. But it's possible that mrdocs needs to be built with BUILD_SHARED_LIBS for plugins to work because the plugin will end up interacting with MrDocs' public API.

I'm not entirely sure what the equivalent of an import library on Linux and macOS is.

Yes. Shared libraries exist on Linux and MacOS. The API for interacting with them is what changes (but that's something to worry about when the plugin system is implemented). Boost.DLL abstracts these differences.

@cppalliance-bot
Copy link

An automated preview of the documentation is available at https://776.mrdocs.prtest2.cppalliance.org/index.html

@Nerixyz
Copy link
Contributor Author

Nerixyz commented Jan 2, 2025

Now, I'm not really sure about this. I think we can only find out once the plugin system is really implemented. But it's possible that mrdocs needs to be built with BUILD_SHARED_LIBS for plugins to work because the plugin will end up interacting with MrDocs' public API.

To "just" support custom generators, exporting the symbols in the executable is enough. That's what enabling MRDOCS_BUILD_SHARED alone does. If the plugins need to be able to run the tool, then it needs to be a shared library.

Yes. Shared libraries exist on Linux and MacOS. The API for interacting with them is what changes (but that's something to worry about when the plugin system is implemented).

Yea, I wasn't sure how dynamic libraries on Linux and macOS would need to be compiled to use the provided functions. But it seems like one can target_link_libraries(library PRIVATE executable) and CMake does the rest.

Boost.DLL abstracts these differences.

LLVM also provides a facility to load dynamic libraries. That's how Clang loads its plugins.

@alandefreitas
Copy link
Collaborator

Yea, I wasn't sure how dynamic libraries on Linux and macOS would need to be compiled to use the provided functions.

In CMake, that's transparent. You just ask CMake for shared libraries and it gives them to you.

LLVM also provides a facility to load dynamic libraries. That's how Clang loads its plugins.

That's nice. I haven't researched that yet, but I thought LLVM would have its version. LLVM comes with its implementation of almost all basic things.

@alandefreitas
Copy link
Collaborator

@Nerixyz Nerixyz force-pushed the fix/msvc-shared branch 2 times, most recently from a94f805 to 6b6979b Compare January 3, 2025 10:41
@cppalliance-bot
Copy link

An automated preview of the documentation is available at https://776.mrdocs.prtest2.cppalliance.org/index.html

@cppalliance-bot
Copy link

An automated preview of the documentation is available at https://776.mrdocs.prtest2.cppalliance.org/index.html

@cppalliance-bot
Copy link

An automated preview of the documentation is available at https://776.mrdocs.prtest2.cppalliance.org/index.html

src/lib/Lib/Config.cpp Outdated Show resolved Hide resolved
@cppalliance-bot
Copy link

An automated preview of the documentation is available at https://776.mrdocs.prtest2.cppalliance.org/index.html

@cppalliance-bot
Copy link

An automated preview of the documentation is available at https://776.mrdocs.prtest2.cppalliance.org/index.html

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.

3 participants