Add CMake build script #77
Conversation
|
@tamasmeszaros Mind to squash those two commits and add proper newlines at EOF? |
Absolutely no. Thank you for considering merging the PR. Should I do it? |
I'm not a maintainer of this repository; just happened to take a look at it because I'm using this library myself. |
|
Hello! It would be nice to have CMake file for the project indeed, as PrusaSlicer project already patches I'm trying to package Thanks! -avp |
|
Also unfortunately in Nix patch there are no tests in |
|
I'm not opposed to including that but I'm not deeply familiar with CMake -- would it be a problem to include it in a I've been horrendously busy with moving and work projects the last couple months, but should be coming up for air soon and plan to scoop up this and some other things in a new release. |
cmake/CMakeLists.txt
Outdated
| set(CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS ON) | ||
|
|
||
| # Add separate libraries for lib instances with static and dynamic memory allocation | ||
| add_library(${PROJECT_NAME} ${srcdir}/heatshrink_decoder.c ${srcdir}/heatshrink_encoder.c) |
There was a problem hiding this comment.
Why not have the suffix be explicit in both cases?
_noalloc vs. dynalloc?
There was a problem hiding this comment.
The naming in the makefile is _static and _dynamic. I find this unfortunate due to potential confusion with static or shared libraries (dynamic linking). If the project is named heatshrink, I would kind of expect that it provides a library with the same name. But yes, then it's hard to know which memory allocation is the library without the suffix using. Maybe I could add an alias to one of them as the default and export that as well.
Maybe this duplicity could also be avoided by using some kind of allocator abstraction. But that is out of the scope for this PR.
There was a problem hiding this comment.
Creating an alias sounds good.
Regarding abstracting the allocator: you'd need LTO for this to work properly and the two libs are a reasonable compromise. Also AFAIK there's some actual code differences in how the memory is used with the dynamic allocation, thus that's not something you could easily abstract away without compromising on the memory footprint.
There was a problem hiding this comment.
Actually while I'm thinking about this library naming, I guess even if the naming is confusing to me, it might still be better to replicate the naming from the original Makefile. Because now, when you build with CMake you get differently named libraries installed compared to what the Makefile produces. It should not matter which build system was used to install the libraries, the result should be the same.
Add heatshrink interface target as default
|
@silentbicycle Please note that the tests fail when building in Debug mode. |
I've put the CMake stuff into cmake subdirectory. I actually forgot that you requested 'contrib' but I would not change it now it you are ok with 'cmake' subdir. I fully understand your perspective about CMake. It is unusual to not place the top level CMake file into the project root and will create confusion, but I think it is managable by any maintainer and is better than not having any CMake build. |
Create aliases for CMake targets with better naming
5 participants
Hello there! I'm sending this PR on behalf of the PrusaSlicer team. We are considering to use heatshrink in our software and put together a CMake build script which supports installing the library and exporting CMake config scripts for downstream projects to be able to find the installed library.