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

Fix the CMake code of the project #85

Open
wants to merge 1 commit into
base: main-dev
Choose a base branch
from

Conversation

friendlyanon
Copy link

Some things this fixes:

  • Never supported CMake 3.1.
  • Did not support clients using CMake.
  • Polluted dependees consuming it via vendoring (i.e. add_subdirectory).
  • Too many pointless genexes. Put the flags in a preset and not the project code.
  • The project was hopelessly broken for MSVC, now the actual library at least builds and can be installed, although stringzillite seems very suspect still. The developer targets are still very much broken.
  • Setting CMAKE_* variables, which is verboten.
  • Pointless noise from those message calls.
  • Not using/supporting CMake provided variables like BUILD_TESTING and CMAKE_SKIP_INSTALL_RULES.

@ashvardanian
Copy link
Owner

This is amazing, @friendlyanon! Thanks! As you see, I'm not a great CMake user 😅

  • What would be a good next step to fix the remaining issues you've highlighted? Should I wait for them before the merge?
  • Does this break some of the documentation I have? If so, it's probably worth updating the README.

@friendlyanon
Copy link
Author

The main goal of this PR is to point out issues and provide solutions to some of them. I would have opened an issue first, but issues can't do that effectively.
Things like fixing MSVC support are beyond this PR and will require more work on your end. I don't have the knowledge to properly address this for example.
CI workflows are also left untouched, since this PR is certainly not a final solution. You would also need to add presets similar to this: [1] [2]

What would be a good next step to fix the remaining issues you've highlighted?

You would definitely need to find a way to acquire a Windows developer environment to deal with MSVC issues. Documenting how to get MSVC to work in a mostly standards compliant fashion would also be a bonus ([3]). I would also like to see the CMake install interface being the only documented way of consuming the project. Supporting a vendoring scenario should merely be a side effect of following CMake best practices with some extra on top (like the ${warning_guard} variable).

Does this break some of the documentation I have?

Certainly. The readme documents both #include <stringzilla.hpp> and #include <stringzilla/stringzilla.hpp>. The install rules solidify #include <stringzilla/stringzilla.hpp>.

As you see, I'm not a great CMake user

No problem, help is readily available. Feel free to tag me for anything CMake related or join the #cmake channel of the C++ Slack.

@friendlyanon
Copy link
Author

Additionally, I would also look into splitting the C "package" into something separate. Seeing how this project is already packaged by Conan, but only the header-only C++ part of it: https://conan.io/center/recipes/stringzilla

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.

None yet

2 participants