-
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
refine Folly recipe #13621
refine Folly recipe #13621
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
7e5fbe2
to
701b479
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
1. add sse42 option projects with simd enabled can not not be linked with folly since sse is not enabled currently. 2. Keep consistent with folly's cmake files. folly itself export components such as follybenchmark the cmake script uses target_link_libraries(xxlib Folly::follybenchmark) works if using manually install folly. but will fail if uses conan's recipe.
I detected other pull requests that are modifying folly/all recipe: This message is automatically generated by https://github.com/ericLemanissier/conan-center-conflicting-prs so don't hesitate to report issues/improvements there. |
recipes/folly/all/conanfile.py
Outdated
@@ -144,6 +150,9 @@ def source(self): | |||
def _configure_cmake(self): | |||
cmake = CMake(self) | |||
if can_run(self): | |||
if self.options.with_sse4_2: |
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.
Since when is it available? Is it backward compatible?
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.
sse4.2 support was in folly long long ago (>7 years)
I will set option default to False to be backward compatible.
recipes/folly/all/conanfile.py
Outdated
} | ||
default_options = { | ||
"shared": False, | ||
"fPIC": True, | ||
"with_sse4_2" : True |
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.
"with_sse4_2" : True | |
"use_sse4_2" : True |
with is not a good prefix, as it's not a dependency. Prefer use
instead: https://github.com/conan-io/conan-center-index/blob/master/docs/packaging_policy.md#recommended-feature-options-names
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.
thanks. fixed.
recipes/folly/all/conanfile.py
Outdated
self.cpp_info.components["folly_test_util"].libs = ["folly_test_util"] | ||
self.cpp_info.components["folly_test_util"].requires = ["libfolly"] | ||
|
||
if Version(self.version) >= "2020.08.10.00" and self.settings.os == "Linux": |
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.
Bad indentation?
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.
not bad indentation.
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.
it is OK to adjust the indentation..
This comment has been minimized.
This comment has been minimized.
recipes/folly/all/conanfile.py
Outdated
@@ -144,6 +151,9 @@ def source(self): | |||
def _configure_cmake(self): | |||
cmake = CMake(self) | |||
if can_run(self): | |||
if self.options.use_sse4_2 and str(self.settings.arch) in ['x86', 'x86_64']: |
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.
if self.options.use_sse4_2 and str(self.settings.arch) in ['x86', 'x86_64']: | |
if self.options.get_safe("use_sse4_2"): |
Let's say we are using ARM arch here. The option use_sse4_2 will not be available and will break conan create command. Checking the arch should be done in validate()
method, like:
def validate(self):
if self.options.get_safe("use_sse4_2") and str(self.settings.arch) not in ['x86', 'x86_64']:
raise ConanInvalidConfiguration(f"{self.ref} can use the option use_sse4_2 only on x86 and x86_64 archs.")
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.
Thanks. fixed.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@uilianries |
This comment has been minimized.
This comment has been minimized.
@kexianda The CI result says: #13621 (comment) Please, remove the line /test_v1_package/conanfile.py:# pylint: skip-file |
This comment has been minimized.
This comment has been minimized.
@@ -103,7 +110,7 @@ def validate(self): | |||
raise ConanInvalidConfiguration("{} requires C++{} support. The current compiler {} {} does not support it.".format( | |||
self.name, self._minimum_cpp_standard, self.settings.compiler, self.settings.compiler.version)) | |||
|
|||
if self.version < "2022.01.31.00" and self.settings.os != "Linux": | |||
if Version(self.version) < "2022.01.31.00" and self.settings.os != "Linux": | |||
raise ConanInvalidConfiguration("Conan support for non-Linux platforms starts with Folly version 2022.01.31.00") | |||
|
|||
if self.settings.os == "Macos" and self.settings.arch != "x86_64": |
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.
Can this finally be updated?
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.
Can this finally be updated?
thanks. fixed now
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.
The new targets are not exported by the project (usually when it changes over versions we just do latest) but some more work is needed if you want this available in older versions
@@ -254,3 +268,30 @@ def package_info(self): | |||
self.cpp_info.components["libfolly"].names["cmake_find_package_multi"] = "folly" | |||
self.cpp_info.components["libfolly"].set_property("cmake_target_name", "Folly::folly") | |||
self.cpp_info.components["libfolly"].set_property("pkg_config_name", "libfolly") | |||
|
|||
if Version(self.version) >= "2019.10.21.00": | |||
self.cpp_info.components["follybenchmark"].set_property("cmake_target_name", "Folly::follybenchmark") |
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.
Where does this come from 🤔 looking at the latest it's not there https://github.com/facebook/folly/search?q=follybenchmark
🤢
at some point this needs to be removed
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.
Where does this come from 🤔 looking at the latest it's not there https://github.com/facebook/folly/search?q=follybenchmark
🤢
at some point this needs to be removed
hi @prince-chrismc
follybenchmark target is still there : https://github.com/facebook/folly/blob/main/folly/CMakeLists.txt#L23
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 clearly did not drink enough coffee.
Perfect thank you for pointign that out!
All green in build 10 (
|
folly
refine folly recipe