-
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
Embree 3.12.0 recipe #354
Embree 3.12.0 recipe #354
Conversation
Sorry, the build is only launched for Early Access Program users. You can request access writing in this issue. |
Hi @KaoCC ! Thanks for your PR! Please, take a look on my comments/review. |
thanks all, I will address these comments in the following commit |
Sorry, the build is only launched for Early Access Program users. You can request access writing in this issue. |
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.
thank you very much ! I addressed them in the following commit.
All green! 😊
|
recipes/embree/all/conanfile.py
Outdated
"backface_culling": False, | ||
"ignore_invalid_rays": False, | ||
"ray_masking": False, | ||
"tbb:tbbmalloc": 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.
As Embree requires this specific option and currently we don't have this package available, we will or remove this option, or provide such configuration as pre-built package.
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.
How would you suggest to do that ? Wouldn't that cause the Embree build to fail if that option is required but 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.
@uilianries Thanks for your comment, would you please provide any guidance of how this PR could proceed ? Is there any change could be made on my side ? 😃
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's a good question. We have similar situation for header-only recipes. We need to provide some option for these cases. @danimtb WDYT ?
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.
Well I thought if the tbb package with that specific option is missing one can simply build it with --build missing
setting. That's how I did on my dev machine.
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.
@KaoCC does this package really need that option of tbb activated? Can you try removing "tbb:tbbmalloc": True,
and see what happens? Thanks
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.
@danimtb ... strange. I just tested it on two distinct machines and it seems like it will also compiled without the flag. I recalled the flag was a requirement. Anyway I removed it and pushed the commit again.
Some configurations of 'embree/3.6.1' have failed:
|
Now it says : ERROR: Missing prebuilt package for 'tbb/2019_u9' |
Yes, sorry about that. I don't know why the error did not show up before. This is the expected behavior for a requirement that has a missing package. Unfortunately, the feature to support specific options will not be immediate but we will prioritize it |
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 terms of quality the recipe looks good, but the ci is failing
Seems the tbb option is required for Mac at least:
|
Some configurations of 'embree/3.6.1' have failed:
|
Some configurations of 'embree/3.6.1' have failed:
|
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.
These changes will fix the MacOS error, I checked on apple-clang 11
Some configurations of 'embree/3.6.1' have failed:
|
I just pushed a fix for TBB, which should solve this current error: #536 |
An unexpected error happened and has been reported. Help is on its way! 🏇 |
An unexpected error happened and has been reported. Help is on its way! 🏇 |
All green in build 35 (
|
cmake.install() | ||
self.copy("LICENSE.txt", src=self._source_folder, dst="licenses") | ||
tools.rmdir(os.path.join(self.package_folder, "share")) | ||
tools.rmdir(os.path.join(self.package_folder, 'lib', 'cmake')) |
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.
What is the config filename, and imported target if any?
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.
which config ?
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 CMake config file generated in os.path.join(self.package_folder, 'lib', 'cmake')
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.
Oh there are lots of them if I remember correctly. I can check it perhaps later.
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.
/lib/cmake/embree-3.12.0/embree-config.cmake
/lib/cmake/embree-3.12.0/embree-config-version.cmake
/lib/cmake/embree-3.12.0/sys-targets.cmake
/lib/cmake/embree-3.12.0/sys-targets-release.cmake
/lib/cmake/embree-3.12.0/math-targets.cmake
/lib/cmake/embree-3.12.0/math-targets-release.cmake
/lib/cmake/embree-3.12.0/simd-targets.cmake
/lib/cmake/embree-3.12.0/simd-targets-release.cmake
/lib/cmake/embree-3.12.0/lexers-targets.cmake
/lib/cmake/embree-3.12.0/lexers-targets-release.cmake
/lib/cmake/embree-3.12.0/tasking-targets.cmake
/lib/cmake/embree-3.12.0/tasking-targets-release.cmake
/lib/cmake/embree-3.12.0/embree_sse42-targets.cmake
/lib/cmake/embree-3.12.0/embree_sse42-targets-release.cmake
/lib/cmake/embree-3.12.0/embree_avx-targets.cmake
/lib/cmake/embree-3.12.0/embree_avx-targets-release.cmake
/lib/cmake/embree-3.12.0/embree_avx2-targets.cmake
/lib/cmake/embree-3.12.0/embree_avx2-targets-release.cmake
/lib/cmake/embree-3.12.0/embree_avx512skx-targets.cmake
/lib/cmake/embree-3.12.0/embree_avx512skx-targets-release.cmake
/lib/cmake/embree-3.12.0/embree-targets.cmake
/lib/cmake/embree-3.12.0/embree-targets-release.cmake
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.
so self.cpp_info.names["cmake_find_package"]
and self.cpp_info.names["cmake_find_package_multi"]
should be embree
. What are the imported targets in *-targets
files?
Co-authored-by: SpaceIm <30052553+SpaceIm@users.noreply.github.com>
Co-authored-by: SpaceIm <30052553+SpaceIm@users.noreply.github.com>
An unexpected error happened and has been reported. Help is on its way! 🏇 |
I've just triggered it manually, I'll monitor it until it finishes... not sure what happened with the previous job. |
An unexpected error happened and has been reported. Help is on its way! 🏇 |
All green in build 41 (
|
tools.rmdir(os.path.join(self.package_folder, "bin")) | ||
|
||
def package_info(self): | ||
self.cpp_info.libs = tools.collect_libs(self) |
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.
From the documentation
EMBREE_STATIC_LIB: Builds Embree as a static library (OFF by default). Further multiple static libraries are generated for the different ISAs selected (e.g. embree3.a, embree3_sse42.a, embree3_avx.a, embree3_avx2.a, embree3_avx512knl.a, embree3_avx512skx.a). You have to link these libraries in exactly this order of increasing ISA.
So you can't simply collect libs.
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.
Thank you very much. I can fix that later, or in a followup patch, when I'm available.
I am all for this being merged but there's two comments which might be worth doing in the future #354 (comment) and #354 (comment) There are 3 approvals but the CCI bot has not merged this one in a few days |
@prince-chrismc indeed, this seems to be a bug in the automatic merge of the bot. Basically, a lot of reviews in this PR that got "hidden" by the github pagination. We are already working on a fix for it. Thank you! |
Bug fixed! Awesome work 🚀 |
Specify library name and version: embree/3.12.0
conan-center hook activated.
Note:
closes #343
tested on local machine with the following compiler: GCC 5.5.0 , Apple Clang 12.0.0