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

WIP - Handle sources as a library + cli app #18

Draft
wants to merge 16 commits into
base: master
Choose a base branch
from

Conversation

piponazo
Copy link
Contributor

With the changes proposed here, CMake would handle the unishox2.h/cpp as a library that is compiled separately from the CLI application.

The main benefit of doing that is that CMake can handle the installation of the library so that other projects can consume the library in the "CMake standard" way. After configuring the project with CMake, one can run make install or similar, to install the library, hedaders and CMake configuration files in the specified installation folder:

PS C:\dev\rfid\Unishox\buildReleaseShared\install> tree /f
Folder PATH listing for volume OSDisk
Volume serial number is 0044-AE03
C:.
├───bin
│       unishox.dll
│
├───include
│   └───unishox
│           unishox2.h
│           unishox_export.h
│
└───lib
    │   unishox.lib
    │
    └───cmake
        └───unishox
                unishoxConfig-release.cmake
                unishoxConfig.cmake
                unishoxConfigVersion.cmake

Another project also configured with CMake can easily consume unishox by just using find_package and then using the unishox target:

# CMake code in other project consuming unishox
find_package(unishox)

add_executable(someApp someApp.cpp)
target_link_libraries(someApp PRIVATE unishox)

The main conflicting point that I see with this PR is that I used the following command to automatically generate an export header:

generate_export_header(unishox)

Such export header would be needed to export/import the library symbols on different platforms and compilers. This is a snippet from the file generate on Windows10 + MSVC:

#ifdef UNISHOX_STATIC_DEFINE
#  define UNISHOX_EXPORT
#  define UNISHOX_NO_EXPORT
#else
#  ifndef UNISHOX_EXPORT
#    ifdef unishox_EXPORTS
        /* We are building this library */
#      define UNISHOX_EXPORT __declspec(dllexport)
#    else
        /* We are using this library */
#      define UNISHOX_EXPORT __declspec(dllimport)
#    endif
#  endif

Previously, the code could be compiled directly with a call to the compiler like this one:

gcc -o unishox2 test_unishox2.c unishox2.c

However, the addition of that export header would make strictly needed a previous configuration of the project with CMake (to generate the export header for the target system). Probably that would be the main point of discussion, in case you want to keep the project independent from any configuration system such as CMake.

siara-cc and others added 13 commits September 13, 2021 10:09
MSVC2019 was complaining about trying to statically allocated the array:

...\test_unishox2.c(377): error C2057: expected constant expression
...\test_unishox2.c(377): error C2466: cannot allocate an array of constant size 0
...\test_unishox2.c(377): error C2133: 'short_buf': unknown size
The size returned by sizeof(buf) was 8 when its length was actually more
than 100 bytes.
@piponazo
Copy link
Contributor Author

By the way, if we agree on continuing on this path, I would redo the PR because the git history in my personal fork is kind of messed up 😅

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.

2 participants