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

Create cmake target #61

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

Conversation

victor1234
Copy link
Contributor

@victor1234 victor1234 commented Apr 24, 2023

  • Create a cmake target for proper installation
  • Update README.md with CMake integration instructions
  • Add cmake find_package(streamvbyte) check to Ubuntu 22.04 CI
  • Set version in cmake according to last tag

@lemire
Copy link
Member

lemire commented Apr 25, 2023

@victor1234 Do you know why it fails under Visual Studio?

@victor1234
Copy link
Contributor Author

PR is not complete now, I need 1-2 days

README.md Outdated
----------------

```
cmake -DCMAKE_BUILD_TYPE=Release ..
Copy link
Member

Choose a reason for hiding this comment

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

We default on release.

Copy link
Member

Choose a reason for hiding this comment

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

Still good to spell it out because under Windows, it must be spelt out.

README.md Outdated
Installation (CMake)
----------------

```
Copy link
Member

Choose a reason for hiding this comment

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

Missing "mkdir build?". Note that you can do cmake -B build ....

@victor1234
Copy link
Contributor Author

@lemire PR is in draft mode now. I'm still working. It's not ready for review.

@victor1234
Copy link
Contributor Author

This is a comprehensive PR and I would like to discuss whether these changes are suitable for your project. The main goal is to create modern cmake targets that install correctly and are available through various integration methods, making it easier to use the library and build packages for it. In addition, global variables are not used in modern cmake (they are propagated to the main project when the library is integrated via add_subdirectory()), and all properties should be assigned to each target specifically. As I understand it, all properties ( sanitizer, ASAN_OPTIONS, __restrict ) should only apply to the streamvbyte target and not propagate to the main project when linking. Only _ARM_NEON__ should be propagated to the example.

Please answer these questions:

  1. Do we need pure Makefile support?
  2. I would not mention DCMAKE_INSTALL_PREFIX in the readme at all, as installation defaults to the standard location where the target can easily found by find_package(streamvbyte).
  3. Which of the compilation, linking and definition flags should only be left with streamvbyte, and which should propagate to the entire project that streamvbyte is linked to?
  4. There is no arm support in github CI, how can I check if the PR works correctly and arm optimization enabled on my raspberry pi4?
  5. I would suggest moving all includes into one folder, it would be #include <streamvbyte/streamvbyte.h>. But this will not be compatible with older versions.

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