joltphysics: add version 5.2.0 #24670
Conversation
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.
|
Hey @ErniGH (or @toge who created the original files) Hopefully you can help me out with this, the main problem I'm having now is that I'm having trouble understanding why patching is necessary, what the current patches do for this recipe, and how to generate my own patches. I read this: https://github.com/conan-io/conan-center-index/blob/master/docs/adding_packages/sources_and_patches.md, but I'm still having trouble seeing why we need a patch in our specific case of this recipe, my specific questions are for the entire patch, but I'll focus on this one first:
|
|
Hi @cuppajoeman thanks for taking the time to add the new version. To be able to answer your questions, we'll need to check the previous PRs, see if some info was discussed there. I'll try to take a look tomorrow, see if we can come up with some answers! :) |
|
@cuppajoeman |
|
@cuppajoeman @AbrilRBS diff --git a/recipes/joltphysics/all/conanfile.py b/recipes/joltphysics/all/conanfile.py
index ef2cbdfc7..bdd5f9e77 100644
--- a/recipes/joltphysics/all/conanfile.py
+++ b/recipes/joltphysics/all/conanfile.py
@@ -1,5 +1,6 @@
from conan import ConanFile
from conan.errors import ConanInvalidConfiguration
+from conan.tools.apple import is_apple_os
from conan.tools.build import check_min_cppstd
from conan.tools.cmake import CMake, CMakeToolchain, cmake_layout
from conan.tools.files import apply_conandata_patches, copy, export_conandata_patches, get
@@ -29,6 +30,7 @@ class JoltPhysicsConan(ConanFile):
"simd": ["sse", "sse41", "sse42", "avx", "avx2", "avx512"],
"debug_renderer": [True, False],
"profile": [True, False],
+ "lto": [True, False],
}
default_options = {
"shared": False,
@@ -36,6 +38,7 @@ class JoltPhysicsConan(ConanFile):
"simd": "sse42",
"debug_renderer": False,
"profile": False,
+ "lto": True,
}
@property
@@ -80,6 +83,8 @@ class JoltPhysicsConan(ConanFile):
del self.options.fPIC
if self.settings.arch not in ("x86", "x86_64"):
del self.options.simd
+ if is_apple_os(self):
+ self.options.lto = False
def configure(self):
if self.options.shared:
@@ -131,6 +136,7 @@ class JoltPhysicsConan(ConanFile):
tc.variables["USE_STATIC_MSVC_RUNTIME_LIBRARY"] = is_msvc_static_runtime(self)
tc.variables["JPH_DEBUG_RENDERER"] = self.options.debug_renderer
tc.variables["JPH_PROFILE_ENABLED"] = self.options.profile
+ tc.variables["INTERPROCEDURAL_OPTIMIZATION"] = self.options.lto
if Version(self.version) >= "3.0.0":
tc.variables["ENABLE_ALL_WARNINGS"] = False
tc.generate() |
|
@shiena Please disable lto if it's manually managed in upstream CMakeList, and don't add such option in conancenter recipe. Lto can be enabled by users through conan config. It's always disabled by default for consistency across libraries provided by conancenter. |
|
@SpaceIm |
|
@shiena Hello and thank you for commenting. Have you discussed with the upstream this situation involving lto, instead? It look pretty particular, and it's configured already to be always ON: https://github.com/jrouwe/JoltPhysics/blob/f95ad217f2c2797081cbc33b6d3463dd0ef65f84/Build/CMakeLists.txt#L29 Maybe the upstream could change the behavior when using OSX to avoid the current error. Otherwise, I would just enforce |
I've made this patch for joltphysics 2.0.1 (first version of joltphysics packaged in conancenter), I don't know if the logic is still relevant for other versions. For 2.0.1, patch was fixing this:
|
|
Just read the issue jrouwe/JoltPhysics#418. I would pass |
|
@uilianries Thank you for all your advice. I will withdraw my request and investigate the upstream topic. |
Signed-off-by: Uilian Ries <[email protected]>
Signed-off-by: Uilian Ries <[email protected]>
|
@cuppajoeman Hello! We are going to review this PR on this Friday (22/11) and check possible solution involving the build. Thank you for your PR again! |
Signed-off-by: Uilian Ries <[email protected]>
|
First, @cuppajoeman Thank for bringing us this new major version of joltphysics in this PR 💚 @TariqYoussef @noizex @shiena Thank for your comments in this PR, it's clear the project has several options and can result in different configurations, as those options are dependent of each other. After discussing about this PR with Conan team, we idealized of reducing this PR, following the minimal of options possible, as the CI can not test all those combinations and may result in future issues. The current configured options for 5.x are to disable examples, tests, profiler and the renderer. In case you really need a specific option, we could discuss in a future issue after this PR, so we would avoid new delays. Still, Conan 2.x offers a safe way to inject cmake definitions without changing any line of the recipe, the configuration This override the value for This configuration does not affect the package ID, so it will override your local cache with a new package that does not follow the recipe in Conan Center. In order affect the package ID, by any configuration used, you can use configuration Reference: https://docs.conan.io/2/reference/config_files/global_conf.html These configuration can be added to your profile as well, so you do not need to copy/paste for every command line. That's a fast way to unblock you in case needing a new option not available in the recipe yet. |
|
The CI was not able to process the current state of this PR. It's an internal bug and is listed to be fixed. Sorry for the inconvenience. |
Is there any idea on the progress of the internal bug or an expected time the bug blocking this will get fixed? Is there an issue we can look at to see the progress of it? |
|
@uilianries Hi, I'm just asking again about any more information or a timeline we can expect the internal bug which is blocking this to be fixed |
|
Hi @cuppajoeman - the bug has been fixed for a while, apologies the job for this has not been relaunched. Please see the Checks tab for CI build results (as there are some failures) |
|
The current error seems to be related to Apple Clang 13 because locally I can build using Apple Clang 16. As I didn't find a related issue on the upstream, so I opened an issue asking for help: jrouwe/JoltPhysics#1482 Once we have the minimal Apple Clang version required, we can add it |
Signed-off-by: Uilian Ries <[email protected]>
…cuppajoeman-master-24670-cii-command
|
The upstream kindly provided a workaround for Apple Clang 13, and the same is merged in the master branch already. I pushed the very same patch to this new recipe version, on the commit 4c278ee |
Signed-off-by: Uilian Ries <[email protected]>
|
Thanks for all the effort to get this package closer to merging 🙇 |
There was a problem hiding this comment.
LGTM.
- The package_info follows the generated cmake values: JoltConfig.cmake.txt
- License is MIT: https://github.com/jrouwe/JoltPhysics/blob/master/LICENSE
- C++ 17 is required: https://github.com/jrouwe/JoltPhysics/blob/v5.2.0/Jolt/Jolt.cmake#L2
- CMake 3.20 is mandatory: https://github.com/jrouwe/JoltPhysics/blob/v5.2.0/Build/CMakeLists.txt#L1
Signed-off-by: Uilian Ries <[email protected]>
|
@uilianries I don't believe that the supplied |
|
@cuppajoeman Thank you for reporting the case in a new issue! It should be investigated during next week! |
|
Yeah I also ran into this issue. The suggested fix works for now! What I wonder is how we can change the different options for this recipe like: via conanfile.txt? It seems like these options are sort of hardcoded and some we may want to have changed for our builds? I noticed I had this in my [options] section of my older Jolt recipe but not sure this ever worked. |
Hello, @noizex! For those exposed options, you can use the Conan 2.x config feature to inject variables directly to CMake when running Conan. For instance:
You can add this configuration to your Conan command line, as represented here, or you can add it to your Conan profile. It will not work in case it is being added to your conanfile.txt. Still, this configuration alone does not affect your package ID, so you will need to add it in another configuration, so Conan will consider any change in
The same can be added to your profile as well. Regards. |
This might already be obvious but something I didn't realize for a while was that if you specify the extra config options, it doesn't by default cause the library to rebuild, you have to pass the I think when you allow for extra variables go be taken into account for the package id that it will automatically trigger the rebuild, and I think once I had to delete my build folder to get it to work. Things to keep in mind for anyone else who visits this. |
Summary
Changes to recipe: joltphysics/[5.0.0]
These changes allow for users to install jolt5.0.0 with conan
Motivation
Many new features were added in jolt 5.0.0, these pertain to soft body, vehicles, character, constraint, collision detection, and can be read more about here: https://github.com/jrouwe/JoltPhysics/releases/tag/v5.0.0
Details
I am still not an authorized user, so I don't think this will be able to be merged in until I am, but I thought I would start the merge request a bit earlier, because it's my first time doing this and thought that there might be things that I have to change after review, by the time it's correct I'd probably have authorization.
WARNING: This merge request is most likely incorrect because I don't fully understand what the previous patch was doing and so I didn't include a patch in this commit, even though running
conan . create --version=5.0.0works just fine.So far I have:
Questions
CMakeLists.txtintest_package:I added this because when it was not there and I tried to run
conan create . --version=5.0.0I got the error:From what I understand this is because we build the
joltphysicspackage in isolation from the test package, since they both use jolt, they must have matching compiler definitions or else the code is not compatible. When I inspect the previous version patches they have lines like this:conan-center-index/recipes/joltphysics/all/patches/3.0.1-0001-fix-cmake.patch
Lines 104 to 108 in 1bbf243
Is my method of changing the
CMakeLists.txtincorrect? Should I be using patching for this? [Also I have read the corresponding section on patches]conan-center-index/recipes/joltphysics/all/patches/3.0.1-0001-fix-cmake.patch
Lines 87 to 102 in 1bbf243
What is the purpose of this modification? Do I need create a patch that does that in my version here?
@toge I know that you created the previous patches, would you be able to help answer a few of the above questions?
build_testing), the options are laid out here. Note that I have read the corresponding section about the conanfile options attributeApologies for asking so many questions, but I really want to help contribute to conan center index. I think once I complete my first merge request I will have far less questions.
Closes: #25558