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

import roaring.h from the right place #658

Merged
merged 2 commits into from
Sep 5, 2024
Merged

Conversation

Dr-Emann
Copy link
Member

@Dr-Emann Dr-Emann commented Sep 5, 2024

We shouldn't be expecting roaring.h to be available in the header search path directly.

@Dr-Emann Dr-Emann requested a review from lemire September 5, 2024 03:49
Copy link
Member

@lemire lemire left a comment

Choose a reason for hiding this comment

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

Looks fine to me. If this was wrong, it would be nice to have a CI test to validate.

Only add `include/`, so includes must be done as `<roaring/xxx.h>`. Also, fix
an issue where `CMAKE_INSTALL_INCDIR` was referenced, the correct variable is
`CMAKE_INSTALL_INCLUDEDIR`:

https://cmake.org/cmake/help/latest/command/install.html
@Dr-Emann
Copy link
Member Author

Dr-Emann commented Sep 5, 2024

Agreed, changed the CMake to only add include/ to the include search path rather than both include/ AND include/roaring/, and verified it would have failed with the updated CMake without this change.

@lemire
Copy link
Member

lemire commented Sep 5, 2024

Merging.

@lemire lemire merged commit 955c769 into master Sep 5, 2024
38 checks passed
@Dr-Emann
Copy link
Member Author

Dr-Emann commented Sep 5, 2024

Recommend another release, believe this could break compiles for users.

@Dr-Emann Dr-Emann deleted the correct_import_source branch September 5, 2024 16:54
@lemire
Copy link
Member

lemire commented Sep 5, 2024

@Dr-Emann Major or minor version?

@Dr-Emann
Copy link
Member Author

Dr-Emann commented Sep 5, 2024

I'd say patch version, it's just a fix, no new functionality or breakage.

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