-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
add marisa/0.2.6 recipe #11877
add marisa/0.2.6 recipe #11877
Conversation
This comment has been minimized.
This comment has been minimized.
ffd6cad
to
da54ce0
Compare
This comment has been minimized.
This comment has been minimized.
da54ce0
to
841f69a
Compare
This comment has been minimized.
This comment has been minimized.
841f69a
to
4197ba7
Compare
This comment has been minimized.
This comment has been minimized.
c2e088a
to
9313ac3
Compare
This comment has been minimized.
This comment has been minimized.
9313ac3
to
342e360
Compare
This comment has been minimized.
This comment has been minimized.
342e360
to
b8f6e68
Compare
This comment has been minimized.
This comment has been minimized.
b8f6e68
to
ed01631
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
--- /dev/null | ||
+++ b/CMakeLists.txt | ||
@@ -0,0 +1,120 @@ | ||
+cmake_minimum_required(VERSION 3.1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DO not create a patch for it, instead, add as regular file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you show me how to do that? Any ref.?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add as you did for other files in this PR:
git add CMakeLists.txt
git commit -s -m "Add Cmake file for marisa"
git push origin marisa/0.2.6
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not use the upstream's build system? https://github.com/s-yata/marisa-trie#build-instructions we have some docs on this https://github.com/conan-io/conan-center-index/blob/master/docs/how_to_add_packages.md#autotools
Also there's a year old PR s-yata/marisa-trie#43 so usually I would suggestion making a PR to share your implementation but they are not very active
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, @prince-chrismc approach is the better way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not use the upstream's build system? https://github.com/s-yata/marisa-trie#build-instructions we have some docs on this https://github.com/conan-io/conan-center-index/blob/master/docs/how_to_add_packages.md#autotools
Also there's a year old PR s-yata/marisa-trie#43 so usually I would suggestion making a PR to share your implementation but they are not very active
The reason why I don't use upstream build system is:
- I want to add build support for Windows MSVC
- Since I keep facing issues like this when I tried to create recipes for
C++ Project
which useautotools
as build system:- Add libstrophe/0.12.1 recipe #11840 kinda has the same issue,
- Add Marisa Trie 0.2.6 #5863 has the same issue.
--> So I tried to use CMake system for this project.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case the "Accepted" approach is something like https://github.com/conan-io/conan-center-index/blob/c4a7e6a366a1e600167da37f40be8cab13357358/recipes/giflib/5.2.x/CMakeLists.txt
There's some points in this CMake, like shared build, which are not required (CMake natively supports that option internal so you dont need to declare it) also there should not be any tests
description = "Matching Algorithm with Recursively Implemented StorAge " | ||
license = ("BSD-2-Clause", "LGPL-2.1") | ||
topics = ("algorithm", "dictionary", "marisa") | ||
exports_sources = "patches/**", "CMakeLists.txt" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There a more optimal way that we are trying to encourage https://github.com/conan-io/conan-center-index/blob/master/docs/reviewing.md#exporting-patches
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I will fix as your suggestion
--- /dev/null | ||
+++ b/CMakeLists.txt | ||
@@ -0,0 +1,120 @@ | ||
+cmake_minimum_required(VERSION 3.1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not use the upstream's build system? https://github.com/s-yata/marisa-trie#build-instructions we have some docs on this https://github.com/conan-io/conan-center-index/blob/master/docs/how_to_add_packages.md#autotools
Also there's a year old PR s-yata/marisa-trie#43 so usually I would suggestion making a PR to share your implementation but they are not very active
self.cpp_info.names["cmake_find_package"] = "marisa" | ||
self.cpp_info.names["cmake_find_package_multi"] = "marisa" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Upstream project does not have CMake targets
self.cpp_info.names["cmake_find_package"] = "marisa" | |
self.cpp_info.names["cmake_find_package_multi"] = "marisa" |
Co-authored-by: Chris Mc <[email protected]>
All green in build 26 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i know it requires some more work, but as this has been open for a while and the recipe is a new one, I prefer to have it accepted than dropping it all the way
marisa
is the dependency oflibeb