Skip to content
This repository has been archived by the owner on Aug 16, 2023. It is now read-only.

Use conan to manage depedency #728

Merged
merged 1 commit into from
Mar 22, 2023
Merged

Use conan to manage depedency #728

merged 1 commit into from
Mar 22, 2023

Conversation

jiaoew1991
Copy link
Collaborator

issue: #727

@mergify mergify bot added the dco-passed DCO check passed. label Mar 9, 2023
conanfile.py Outdated
self.requires("nlohmann_json/3.11.2")
self.requires("llvm-openmp/12.0.1")
if self.options.with_ut:
self.requires("gtest/1.8.1")
Copy link
Contributor

Choose a reason for hiding this comment

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

seems previous we use gtest 1.12, AIK gtest<=1.10 may have some compilation problems for the new compiler.

@matrixji
Copy link
Contributor

A general discussion about how to use knowhere from the developer's view, recently I'm thinking of this for milvus-sdk-cpp also.

As knowhere is a library, it's not fully like milvus, so developers may use knowhere as a component and integrate it into their own project, for milvus currently most developers will only use the final binary.
So as a developer if they could fetch knowhere from conan.io, that's fine, it's pretty good. But someone may want to keep the original way(just use cmake), so if possible, IMO, we'd better provide conan as an option, but not a mandatory thing.

Just like grpc, when I go through grpc-cpp's README it tells me grpc could be imported by Bazal, Cmake, vcpkg and also conan(which not documented in README)

@mergify mergify bot added ci-passed and removed ci-passed labels Mar 16, 2023
)
FetchContent_MakeAvailable(nlohmann_json)
endif()
set(CMAKE_CXX_STANDARD 17)
Copy link
Collaborator

Choose a reason for hiding this comment

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

no need set CMAKE_CXX_STANARD again.

if(WITH_RAFT)
include(cmake/libs/libraft.cmake)
endif()

find_package(nlohmann_json)
Copy link
Collaborator

Choose a reason for hiding this comment

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

split this into cmake/lib as a nlohmann_json.cmake maybe looks better.

Signed-off-by: Enwei Jiao <enwei.jiao@zilliz.com>
@mergify mergify bot added ci-passed and removed ci-passed labels Mar 22, 2023
@liliu-z
Copy link
Member

liliu-z commented Mar 22, 2023

/lgtm
/approve

@sre-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jiaoew1991, liliu-z

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@sre-ci-robot sre-ci-robot merged commit 8694b03 into milvus-io:main Mar 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants