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

Draft: VCPKG rework #781

Open
wants to merge 102 commits into
base: main
Choose a base branch
from
Open

Draft: VCPKG rework #781

wants to merge 102 commits into from

Conversation

dudoslav
Copy link
Contributor

UNDER DEVELOPMENT

This matches the Core and libtiledbsoma. In particular we need at least 3.15 because of a policy that picks the correct CRT linkage on Windows. Also recent CMake versions will warn if the version is set to earlier than 3.10.
With this, `tiledbvcf-bin` compiles and links successfully on Windows.
Comment on lines +284 to +285
RUNTIME DESTINATION tiledbvcf
LIBRARY DESTINATION tiledbvcf/lib
Copy link
Member

Choose a reason for hiding this comment

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

We should be using bin and lib.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Definitely, after the SEGFAULT is fixed I'll clean up the changes to be more organized and use CMAKE_* variables

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is kind of an issue, since CMAKE_INSTALL_LIBDIR points to site-packages/lib which is not correct for our case. Looking at other projects they also use just folder names: https://github.com/ssciwr/clang-format-wheel/blob/dc29f5e2a074dee8e862eb29fd0de1d84f41707a/CMakeLists.txt#L43

I don't think that we need to use GNUInstallDirs CMake module. However, perhaps this will come into play when I'll be testing Conda packaging.

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