Feature/add instinct cpp #24480
Feature/add instinct cpp #24480
Conversation
|
|
This comment has been minimized.
This comment has been minimized.
|
I think the access request is approved. How should we proced this PR? |
This comment has been minimized.
This comment has been minimized.
|
@RobinQu Sorry for the delay on the review: I'll push some cleanup changes to your recipe and comment on them in a bit :) |
| self.requires("reactiveplusplus/2.1.1", transitive_headers=True) | ||
| self.requires("icu/74.1") | ||
| self.requires("tsl-ordered-map/1.1.0", transitive_headers=True) | ||
| self.requires("fmt/10.2.1", transitive_headers=True) |
There was a problem hiding this comment.
The transitive_headers would only be needed if those includes were part of the public API of the project, usually there are a few, but it's a bit suprising that every dependency has it. Does every dependency leak to the consumers?
| if self.dependencies[dep].options[option] != value: | ||
| raise ConanInvalidConfiguration(f'{self.ref} needs -o="{dep}/*:{option}={value}"') | ||
|
|
||
| _ensure_dep_has_option("duckdb", "with_httpfs", self.default_options["duckdb/*:with_httpfs"]) |
There was a problem hiding this comment.
This is bad news if it's not the default for those packages: It will make it so nothing gets built in our CI, as every configuration would still not find these packages - cci only builds the default options for each package (minus shared/header_only options where we combine them). If these are not the defaults, we'll need successful compilation logs of a few different configurations before we're confident about merging this
I'll provide apple-clang ones once the recipe is ready to go to save you some time :)
| get(self, **self.conan_data["sources"][self.version], strip_root=True) | ||
|
|
||
| def generate(self): | ||
| tc = CMakeToolchain(self) |
There was a problem hiding this comment.
A bit suprising for a library with so many dependencies to have these little amount of options, it's great! :)
|
|
||
| self.requires("llama-cpp/b3040", transitive_headers=True) | ||
| self.requires("cpp-httplib/0.15.3", transitive_headers=True) | ||
| self.requires("cli11/2.4.1") |
There was a problem hiding this comment.
I had to remove the gtest test_require from here:
If we want to give users the ability to run the tests, we have a way to control that with the skip_tests conf, but usually CCI packages simply just don't build/run them, so no need to add test_requirements :)
| return { | ||
| "apple-clang": "15", | ||
| "clang": "17", | ||
| "gcc": "12" |
There was a problem hiding this comment.
Has this library been tested on Windows? we would add a min msvc if it's possible to run there :)
| "with_duckdb": True, | ||
| "with_exprtk": True, | ||
| "with_pdfium": True, | ||
| "with_duckx": True, |
There was a problem hiding this comment.
It would also be great to have successful compilation logs when turning off some or all of these options, just to check that it works :)
This comment has been minimized.
This comment has been minimized.
Conan v1 pipeline ❌Failure in build 6 (
Note: To save resources, CI tries to finish as soon as an error is found. For this reason you might find that not all the references have been launched or not all the configurations for a given reference. Also, take into account that we cannot guarantee the order of execution as it depends on CI workload and workers availability. Conan v2 pipeline ✔️
All green in build 6 (
|
|
@AbrilRBS Thanks for your response. I will look into these issues after a 4-day vocation. |
|
Hope you have a good one :) Ping me once you address them/if you need any help! |
Summary
Changes to recipe: instinct-cpp/0.1.5
Motivation
Make instinct-cpp available on conan center
Details
First recipe of instinct-cpp v0.1.5