Recent commit causes issues on Windows, macOS and Linux

[leonstyhre]

Hi!

The commit b7e72fb breaks my build with LunaSVG on Windows as the .dll file is now placed inside a bin subdirectory and the .lib file inside a lib subdirectory instead of being saved to the root of the build tree as was previously the case.

Would it be possible to revert to the previous locations so builds continue to work as before?
Thanks!

[sammycage]

@seanharmer?

[sammycage]

#84

[seanharmer]

Could you elaborate on what you mean by "breaks my build" please? What precisely breaks?

[leonstyhre]

Hi, I actually just realized that the build is broken not only on Windows but on Linux, BSD Unix and macOS too, so I changed the issue description.

I have a number of dependencies for my application and when built they all end up in the root of the build tree so they can be used by my application (and be linked for the platforms where I use static linking). However with the commit mentioned above LunaSVG is now an exception to this as the files are instead saved into bin and lib subdirectories. This makes it impossible to run and package the application as the .a and .dll and .lib files are no longer in the same location as the application binary.

If you really need to move these files into separate subdirectories for some reason, then please provide a mechanism (e.g. a CMake option) for retaining the old logic so LunaSVG works as all the other libraries.

With the latest commits I'm also seeing some new compiler warnings on Windows when building with MSVC that were not present previously, although I don't understand precisely what they mean:

C:\Programmering\EmulationStation-DE_MSVC\emulationstation-de\external\lunasvg\include\lunasvg.h(121): warning C4251: 'lunasvg::Bitmap::m_impl': class 'std::shared_ptr<lunasvg::Bitmap::Impl>' needs to have dll-interface to be used by clients of class 'lunasvg::Bitmap'
C:\Programmering\EmulationStation-DE_MSVC\emulationstation-de\external\lunasvg\include\lunasvg.h(121): note: see declaration of 'std::shared_ptr<lunasvg::Bitmap::Impl>'
C:\Programmering\EmulationStation-DE_MSVC\emulationstation-de\external\lunasvg\include\lunasvg.h(208): warning C4251: 'lunasvg::Document::root': class 'std::unique_ptr<lunasvg::LayoutSymbol,std::default_delete<lunasvg::LayoutSymbol>>' needs to have dll-interface to be used by clients of class 'lunasvg::Document'
C:\Programmering\EmulationStation-DE_MSVC\emulationstation-de\external\lunasvg\include\lunasvg.h(208): note: see declaration of 'std::unique_ptr<lunasvg::LayoutSymbol,std::default_delete<lunasvg::LayoutSymbol>>'

When building with MinGW these warnings are not generated.

[leonstyhre]

Hi, any news regarding this?

Unfortunately the ES-DE AUR release is already broken on Arch Linux as it now uses GCC 13. This means that it already affects a number of users that can't update or install my application. To fix that I need commit db27b7c but I can't update to that, as the commit discussed in this issue breaks the build.

I would really need a fix as soon as possible so that I can make it possible to build ES-DE on Arch Linux again. As described I need the artifacts to end up in the root of the repository and not in subdirectories.
Thanks!

[sammycage]

Did this commit db27b7c break the build as well? I will look into this issue in a month's time; I am kind of busy right now.

[leonstyhre]

No it's the CMake change that breaks the build:
b7e72fb

- if(BUILD_SHARED_LIBS)
-     target_compile_definitions(lunasvg PUBLIC LUNASVG_SHARED)
-     target_compile_definitions(lunasvg PRIVATE LUNASVG_EXPORT)
+ set_target_properties(lunasvg
+     PROPERTIES
+     ARCHIVE_OUTPUT_DIRECTORY "${CMAKE_BINARY_DIR}/lib"
+     LIBRARY_OUTPUT_DIRECTORY "${CMAKE_BINARY_DIR}/lib"
+     RUNTIME_OUTPUT_DIRECTORY "${CMAKE_BINARY_DIR}/bin"

LunaSVG itself compiles but liblunasvg.a (and .dll and .lib) ends up in subdirectories as you can see above.

I can't wait a month, I can't even wait a day really as my application now can't be installed via the AUR. So it affects production right now. If you don't have time then please simply revert the CMake patch and everything should work correctly again.

[sammycage]

Okay, I understand your point now. However, it seems like a personal issue. Maybe you should try using the following command in order to address the problem:

cmake -B <build_directory> -DCMAKE_ARCHIVE_OUTPUT_DIRECTORY=<archive_output_directory> -DCMAKE_LIBRARY_OUTPUT_DIRECTORY=<library_output_directory> -DCMAKE_RUNTIME_OUTPUT_DIRECTORY=<runtime_output_directory> <path_to_source>

This command can help you specify the desired output directories for your build process.

[leonstyhre]

What you propose will not work, but the following workaround/override does work:

add_subdirectory(lunasvg EXCLUDE_FROM_ALL)

set_target_properties(lunasvg
    PROPERTIES
    ARCHIVE_OUTPUT_DIRECTORY ${PROJECT_SOURCE_DIR}
    LIBRARY_OUTPUT_DIRECTORY ${PROJECT_SOURCE_DIR}
    RUNTIME_OUTPUT_DIRECTORY ${PROJECT_SOURCE_DIR}
)

That's in my CMakeLists.txt file so I can do this without touching the LunaSVG subtree.

It's however very crazy that I need to do this, on Unix/Linux and macOS I build using static linking and I can't believe anyone would want to have the liblunasvg.a file in a lib subdirectory, you want to have all artifacts in the same build directory when compiling libraries in-tree as many projects do. All other libraries behave this way and LunaSVG did as well prior to the commit discussed in this issue.

If using dynamic linking like I do on Windows it's even worse as the application will not run from the build directory as the .dll file is not in the same directory as the application binary. That makes it impossible to debug for example, again I can't understand why anyone would want that behavior.

But as the workaround described above solves the problem for me on all supported platforms it's a non-issue for me now. It will however be very confusing for anyone else that attempts to build in-tree using your library, unless they find this issue of course and can read about the workaround.

Btw, this still does not solve the MSVC compiler warnings mentioned in a previous comment, but since I only use MSVC for testing and debugging and make all releases using MinGW this is not a practical problem for me and like mentioned in a previous comment I don't even understand exactly what the message means anyway and what is its implications.

With all this said I'll now be able to at least make my application compile correctly with GCC 13 so the urgent issue will be resolved.

[sammycage]

Hey @leonstyhre,

I apologize for the delay in reverting the commit. I understand that the issue with the CMake not working for you is causing inconvenience. The reason for reverting the commit is that @seanharmer, who made the changes, is not responding to the issue raised.

In order to ensure stability and functionality for everyone, I have decided to revert the commit for now. Rest assured, I will personally take responsibility for "modernizing" the CMake build as soon as I have the time to do so.

Thank you for bringing this issue to my attention, and I apologize again for any inconvenience caused. If you have any further concerns or questions, please let me know.

Best regards,
Samuel Ugochukwu

[lcgamboa]

Hi @sammycage,

Now my projects build scripts and github CI are broken again. When there was the first change in Cmake files I simply updated all the scripts on my projects to fetch the library in the new path and everything worked normally. Now I have to undo everything because someone else can't set up their own project. I hope there are no more such changes. I personally do not agree with this regression.

[sammycage]

Hi @lcgamboa,

I apologize for the inconvenience caused by the recent changes to the CMake files. The decision to revert the commit was not solely based on the output directory but also due to a warning related to 'lunasvg_export.h' that was affecting the build.

I understand your frustration in having to undo your project configurations due to someone else's difficulties. However, it's important to address issues that impact the overall stability and maintainability of the codebase.

Regarding the CMake script itself, It does appear to be quite complex and might be considered as over-engineering. It's important to strike a balance between functionality and maintainability. It's crucial for me to create code that I can effectively maintain in the long run. I will make sure to address these issues and find a more suitable solution that doesn't disrupt existing projects.

Thank you for expressing your concerns, and I'll make sure to take them into consideration for future changes. If you have any further questions or suggestions, please don't hesitate to let me know.

Best regards,
@sammycage

[leonstyhre]

Thanks @sammycage for reverting the commit and for looking into modernizing the configuration in a more non-disruptive manner.

To clarify though I did manage to get my application to build with the problematic CMake changes using the workaround described in an earlier comment. I will probably await your planned CMake updates before pulling any further LunaSVG updates into my project so I don't need to change my CMake configuration twice more. At the moment it's working fine so there's no urgency when it comes to this for my project at least (I'm using LunaSVG as a subtree and only pull upstream updates manually when needed).

Thanks as always for your excellent support and for your very nice SVG library.

[dg0yt]

Hm, reverting the whole change, including the exported CMake config, seem quite a big step back just to restore build-internal target properties. The broken use case was tight coupling, and the CMake config is the enabler for loose coupling.

[dg0yt]

Do I understand correctly that there is no progress with regard to exported CMake config?