Skip to content

Make sai library shared library, add install directives#21

Merged
Wunkolo merged 2 commits into
Wunkolo:mainfrom
luspi:main
Nov 18, 2025
Merged

Make sai library shared library, add install directives#21
Wunkolo merged 2 commits into
Wunkolo:mainfrom
luspi:main

Conversation

@luspi
Copy link
Copy Markdown
Contributor

@luspi luspi commented Sep 10, 2025

Hi,

Thanks for providing this library!

I'm working on adding libsai support to my application, and for that needed to adjust the install process to also build a shared library and to install the files into the respective locations. This adds to CMakeLists.txt the SHARED keyword to the add_library command and provides install directives for the executables, the header file, and the library itself.

@Wunkolo
Copy link
Copy Markdown
Owner

Wunkolo commented Nov 11, 2025

Oh gosh sorry I managed to miss this for all this time, I'll get this merged soon!

@Wunkolo
Copy link
Copy Markdown
Owner

Wunkolo commented Nov 13, 2025

This looks good for a re-usable library, though I don't think this should be the default behavior and should instead be a CMake option of some kind. The Thumbnail, Decrypt, Tree, and Document are more like unit-tests and were never intended to be shipped user-facing binaries either 💦.

Does the library need to be shared in particular? I'd feel better about it just having to install a static library file(libsai.a) and the header file.

Copy link
Copy Markdown
Contributor

@BLumia BLumia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm obviously not the author of libsai but just a random user, but why not make use of the CMake BUILD_SHARED_LIBS option? Hardcode it as a shared library is probably not the best option I guess?

In general we can put something like this in the CMakeLists.txt file, and make it defaults to ON if we want shared libs by default or OFF if otherwise:

option(BUILD_SHARED_LIBS "Prefer to build shared lib" ON)

And for installation, it could be better if we could reuse GNUInstallDirs too.

@luspi
Copy link
Copy Markdown
Contributor Author

luspi commented Nov 18, 2025

Thanks for the comments. I added a new commit to this pull request that:

  • uses a BUILD_SHARED_LIBS option to toggle a shared build (off by default to match the current state of the code)
  • uses include(GNUInstallDirs) to install the targets (I wasn't aware of GNUInstallDirs before, I'll use it in my own projects from now on too; thanks!)
  • removes the installation of the binaries (since they, as you said, only really serve as unit tests)

@Wunkolo
Copy link
Copy Markdown
Owner

Wunkolo commented Nov 18, 2025

Looks good to me! Thank you both for your insight and contribution 🙏

@Wunkolo Wunkolo merged commit df66c16 into Wunkolo:main Nov 18, 2025
5 checks passed
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