SharedStaticStarter icon indicating copy to clipboard operation
SharedStaticStarter copied to clipboard

export.h gets overwritten when installing shared and static targets into same prefix

Open lepus2589 opened this issue 3 years ago • 6 comments

Hi Alex,

I recently discovered your blog post about Building a Dual Shared and Static Library while trying to figure out the same thing. It was very helpful, thank you!

I encountered one problem, though, which is either a bug or a lack of understanding on my part.

I cloned this example project and built and installed it exactly like you did in the blog.

$ cmake -S . -B build-shared -DBUILD_SHARED_LIBS=YES -DCMAKE_BUILD_TYPE=Release
$ cmake -S . -B build-static -DBUILD_SHARED_LIBS=NO  -DCMAKE_BUILD_TYPE=Release
$ cmake --build build-shared
$ cmake --build build-static
$ cmake --install build-shared --prefix ./bin
$ cmake --install build-static --prefix ./bin

After examining the installation directory, I noticed that, depending on which target was installed first, the export.h file was different. It gets overwritten on each install.

When generated for the shared target, it looks fine. The __attribute__((visibility("default"))) expressions are present. When generated for the static target, these expressions are missing. All macros are empty then. When studying the GenerateExportHeader docs, this also works exactly, how it is described.

In my understanding, the export.h file should always look like the one for the shared target. When building the static target, the SOMELIB_STATIC_DEFINE should take care of defining the empty macros. But to make this work, one would have to create some kind of dummy shared target in addition to the static target, I guess.

What's your take on this?

Kind Regards

lepus2589 avatar Jan 18 '22 08:01 lepus2589

Yuck... this is a real bug. I'm not sure what the best solution is. Three options jump to mind, in no particular order:

  1. We could call this a CMake bug and try to work with the maintainers to:
    1. Always have generate_export_header generate the same file regardless of library type
    2. Have generate_export_header add the static define to the target, guarded by a generator expression that checks the library type.
  2. We could say that when using this approach, you have to install the shared library second because of the aforementioned bug.
    1. I agree it shouldn't have to be this way, but alas.
    2. It might be possible to use install(CODE) to check that the static install doesn't overwrite a shared export.h.
  3. We could have each build generate a separate export header: export_static.h and export_shared.h. An intermediate header export.h would dispatch between the two depending on whether SOMELIB_STATIC_DEFINE is set.
    1. Yes this is horribly redundant.
    2. On the other hand, it's tolerant to other divergences between the two header sets that CMake might (misguidedly, imo) introduce in the future.
    3. The intermediate header is fixed and so is safe to overwrite.

Thanks for catching this, by the way! :slightly_smiling_face:

alexreinking avatar Jan 18 '22 08:01 alexreinking

Thanks for the fast reply! I came up with this solution for the time being:

macro (generate_export_header_shared _target_name _export_header_relative_path)
    if (NOT EXISTS "${CMAKE_CURRENT_BINARY_DIR}/${_export_header_relative_path}")
        include(GenerateExportHeader)

        message(CHECK_START "Generating export header")

        get_target_property("_${_target_name}_SOURCES" "${_target_name}" SOURCES)
        add_library(__ExportDummy__ SHARED)
        target_sources(__ExportDummy__ PRIVATE "${_${_target_name}_SOURCES}")
        set_target_properties(
            __ExportDummy__
            PROPERTIES
            DEFINE_SYMBOL "${_target_name}_EXPORTS"
            EXCLUDE_FROM_ALL True
            EXCLUDE_FROM_DEFAULT_BUILD True
        )

        # This header contains macros to define ABI visibility of functions
        generate_export_header(
            __ExportDummy__
            BASE_NAME "${_target_name}"
            EXPORT_FILE_NAME "${_export_header_relative_path}"
        )

        message(CHECK_PASS "done")
    endif ()
endmacro ()

It basically creates a dummy shared target, copies over all the sources from the actual target and excludes the dummy from all possible builds.

But I still think, this should be escalated to the CMake devs, somehow.

lepus2589 avatar Jan 18 '22 12:01 lepus2589

To comment on your suggestions:

  1. We could call this a CMake bug and try to work with the maintainers to:
    1. Always have generate_export_header generate the same file regardless of library type
    2. Have generate_export_header add the static define to the target, guarded by a generator expression that checks the library type.

For me, this should be the solution. It reduces boilerplate code and just works as expected, no matter what target it is used on.

  1. We could say that when using this approach, you have to install the shared library second because of the aforementioned bug.
    1. I agree it shouldn't have to be this way, but alas.
    2. It might be possible to use install(CODE) to check that the static install doesn't overwrite a shared export.h.

Relying on the user of your library having to respect the correct order of install sounds just wrong and would probably fail for many users.

To 2.2: If the user for some reason only builds and installs a static variant, the header would still have to be created and installed correctly.

  1. We could have each build generate a separate export header: export_static.h and export_shared.h. An intermediate header export.h would dispatch between the two depending on whether SOMELIB_STATIC_DEFINE is set.
    1. Yes this is horribly redundant.
    2. On the other hand, it's tolerant to other divergences between the two header sets that CMake might (misguidedly, imo) introduce in the future.
    3. The intermediate header is fixed and so is safe to overwrite.

This might be a robust, if redundant, solution if the CMake devs won't fix this when brought to their attention.

I came up with the above macro solution in the meantime.

lepus2589 avatar Jan 18 '22 12:01 lepus2589

For me, this should be the solution. It reduces boilerplate code and just works as expected, no matter what target it is used on.

I agree, though it wouldn't solve anything for users of earlier versions.

To 2.2: If the user for some reason only builds and installs a static variant, the header would still have to be created and installed correctly.

Which it does already if only the static variant is built. My suggestion is that the static build only should check if the export header already exists (because the shared variant is already installed) and then refuse to overwrite it.

This might be a robust, if redundant, solution if the CMake devs won't fix this when brought to their attention.

This might be somewhat shorter & simpler to maintain than the macro. If you have time before I do, please open an issue with Kitware/CMake upstream. Otherwise, I can get to it sometime next week.

alexreinking avatar Jan 20 '22 20:01 alexreinking

Upstream issue has been opened: https://gitlab.kitware.com/cmake/cmake/-/issues/23195

lepus2589 avatar Feb 08 '22 11:02 lepus2589

It feels weird to have different headers for different usage of shared/static libraries. If there are any changes it should be already compiled out, not have it present in the api header. Otherwise downstream has to implement different interface as well depending on how the library is built. This would be even more problematic when SomeLib_SHARED_LIBS is specified at the end of a chain, e.g. A->B->C and when building A we pass -DC_SHARED_LIBS=OFF and B has to adjust its implementation accordingly.

I think this use-case should rather be flagged as an inappropriate design. Would be curious to see real world applications where different headers have to be defined for shared/static libraries.

LecrisUT avatar Feb 16 '23 15:02 LecrisUT