joltphysics: add version 5.0.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. |
This comment has been minimized.
This comment has been minimized.
This is actually an interesting question - most of the packages in Conan repository state only a few recent versions, often just a couple of minor versions within single major version. I browsed existing recipes looking for a jump from a very basic version like 1.0 to 3.0 or higher, something that would also experience a change in make configs, addition of new flags and options - and couldn't find anything. Most of packages just have a static set of options. If there was such a huge change between v3 and v5 of Jolt and it introduced a lot of new options compared to the older versions, I actually started to wonder if this does not justify a separate recipe for newer version? But couldn't find such use cases either. I was able to find some hints in Conan docs for options here It notes some way to dynamically remove options based on some conditional checks, so I think this could be used to check against the
|
|
TLDR; Despite UPDATE: Hmm, there will still be problem with
This is true, but in my opinion package built with Here is how options are defined in Conan for "profiler": False, # so long as the following two options are false, this variable comes into effect,
# this is added as a custom conan option so that you can have custom profiler
# behavior
"profiler_in_debug_and_release": True,
"profiler_in_distribution": False,
<...>
tc.variables["PROFILER_IN_DEBUG_AND_RELEASE"] = self.options.profiler_in_debug_and_release
tc.variables["PROFILER_IN_DISTRIBUTION"] = self.options.profiler_in_distribution
<...>
# if both options are false
if (not self.options.profiler_in_debug_and_release and not self.options.profiler_in_distribution):
# but you want custom behavior
if (self.options.profiler):
self.cpp_info.defines.append("JPH_PROFILE_ENABLED")This looks correct to me as these are the defaults from Jolt CMake config as well: # Enable the profiler in Debug and Release builds. Note that PROFILER_IN_DISTRIBUTION will override this setting.
option(PROFILER_IN_DEBUG_AND_RELEASE "Enable the profiler in Debug and Release builds" ON)
# Enable the profiler in all builds.
# Note that enabling this reduces the performance of the library.
option(PROFILER_IN_DISTRIBUTION "Enable the profiler in all builds" OFF)There is also this note: PROFILER_IN_DISTRIBUTION will override this setting which sort of indicates that But here is how it's handled in Jolt's CMake: # Enable the profiler
if (PROFILER_IN_DISTRIBUTION)
target_compile_definitions(Jolt PUBLIC "JPH_PROFILE_ENABLED")
elseif (PROFILER_IN_DEBUG_AND_RELEASE)
target_compile_definitions(Jolt PUBLIC "$<$<CONFIG:Debug,Release,ReleaseASAN,ReleaseUBSAN>:JPH_PROFILE_ENABLED>")
endif()This condition does not actually make I think this is mistake in upstream on how it handles this condition, at least based on the above Note that tells us that the more general setting is "overriding". I think it either needs to set # Enable the profiler
if (PROFILER_IN_DISTRIBUTION)
if (PROFILER_IN_DEBUG_AND_RELEASE)
target_compile_definitions(Jolt PUBLIC "$<$<CONFIG:Debug,Release,ReleaseASAN,ReleaseUBSAN>:JPH_PROFILE_ENABLED>")
else()
target_compile_definitions(Jolt PUBLIC "JPH_PROFILE_ENABLED")
endif()
endif()which would I think match what the intention behind this flag is (again, assuming this from the Note that the more general setting should override the more specific one - so |
|
Hi @noizex I looked over what you wrote here and I agree with most of what was said. I have some notes and a question: I did some reading of the docs and found this out: https://docs.conan.io/2/tutorial/versioning/revisions.html which can be summed up by:
So as I understand it, it might not be necessary to conditionally load in specific options because for each version, the most recent revision is used for that version, though that would almost nullify the reason to check for version which might explain why you're not seeing many examples of it, though there may be cases that it's used, but as I mentioned I don't have enough experience with conan to know those cases yet. As you mentioned the current status of this package is that we get mismatched defines, and I don't see where the discrepancy comes from, we have: "profiler": False, # so long as the following two options are false, this variable comes into effect,
# this is added as a custom conan option so that you can have custom profiler
# behavior
"profiler_in_debug_and_release": True,
"profiler_in_distribution": False,
<...>
tc.variables["PROFILER_IN_DEBUG_AND_RELEASE"] = self.options.profiler_in_debug_and_release
tc.variables["PROFILER_IN_DISTRIBUTION"] = self.options.profiler_in_distributionwhich would equate to setting: So when we build the package using Note: On second thought I think the logic is right in the if statement because suppose that TLDR: From what I can see everything seems correct but we're getting the mismatched defines message, if we can figure that out we should be all the way there (hopefully) |
Conan v1 pipeline ❌Failure in build 12 (
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 ❌
The v2 pipeline failed. Please, review the errors and note this is required for pull requests to be merged. In case this recipe is still not ported to Conan 2.x, please, ping Failure in build 11 (
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. |
|
Hi @noizex I saw your slack thread where it was mentioned that you would want to directly append the the definition, but I'm still curious we have lines like this in conanfile.py: tc.variables["DEBUG_RENDERER_IN_DEBUG_AND_RELEASE"] = self.options.debug_renderer_in_debug_and_releaseand then in jolt.cmake: # Enable the debug renderer
if (DEBUG_RENDERER_IN_DISTRIBUTION)
target_compile_definitions(Jolt PUBLIC "JPH_DEBUG_RENDERER")
elseif (DEBUG_RENDERER_IN_DEBUG_AND_RELEASE)
target_compile_definitions(Jolt PUBLIC "$<$<CONFIG:Debug,Release,ReleaseASAN,ReleaseUBSAN>:JPH_DEBUG_RENDERER>")
endif()From what I understand when you set tc.variables you're defining those options to be true which would then allow one of these branches to execute in turn creating the compiler definition? |
I think that explains why I had this lingering thought that appending to defines is not something that solves every situation. I suppose this would require literally recreating this logic from library's CMakeFiles to Conan logic in |
Yes, this passes that flag to CMake and then CMake adds the compile definition based on this. But to make it reach the consumer of the package, based on that Slack conversation, we need to add this to |
|
@noizex I have an idea on how we can do this something like this: conan_option_to_cmake_option_name = {
"use_asserts": "USE_ASSERTS",
"double_precision": "DOUBLE_PRECISION",
"generate_debug_symbols": "GENERATE_DEBUG_SYMBOLS",
"override_cxx_flags": "OVERRIDE_CXX_FLAGS",
"cross_platform_deterministic": "CROSS_PLATFORM_DETERMINISTIC",
"cross_compile_arm": "CROSS_COMPILE_ARM",
"build_shared_libs": "BUILD_SHARED_LIBS",
"interprocedural_optimization": "INTERPROCEDURAL_OPTIMIZATION",
"floating_point_exceptions_enabled": "FLOATING_POINT_EXCEPTIONS_ENABLED",
"cpp_exceptions_enabled": "CPP_EXCEPTIONS_ENABLED",
"cpp_rtti_enabled": "CPP_RTTI_ENABLED",
"object_layer_bits": "OBJECT_LAYER_BITS",
# X86 processor features
"use_sse4_1": "USE_SSE4_1",
"use_sse4_2": "USE_SSE4_2",
"use_avx": "USE_AVX",
"use_avx2": "USE_AVX2",
"use_avx512": "USE_AVX512",
"use_lzcnt": "USE_LZCNT",
"use_tzcnt": "USE_TZCNT",
"use_f16c": "USE_F16C",
"use_fmadd": "USE_FMADD",
# WebAssembly SIMD
"use_wasm_simd": "USE_WASM_SIMD",
# Warnings and Stats
"enable_all_warnings": "ENABLE_ALL_WARNINGS",
"track_broadphase_stats": "TRACK_BROADPHASE_STATS",
"track_narrowphase_stats": "TRACK_NARROWPHASE_STATS",
# Debug Renderer
"debug_renderer_in_debug_and_release": "DEBUG_RENDERER_IN_DEBUG_AND_RELEASE",
"debug_renderer_in_distribution": "DEBUG_RENDERER_IN_DISTRIBUTION",
# Profiler
"profiler_in_debug_and_release": "PROFILER_IN_DEBUG_AND_RELEASE",
"profiler_in_distribution": "PROFILER_IN_DISTRIBUTION",
# Allocator and Vector
"disable_custom_allocator": "DISABLE_CUSTOM_ALLOCATOR",
"use_std_vector": "USE_STD_VECTOR",
# Object Stream and Install
"enable_object_stream": "ENABLE_OBJECT_STREAM",
"enable_install": "ENABLE_INSTALL"
}
this pretty much would copy the settings for both the
def custom_profiler_check(options):
return not options.profiler_in_debug_and_release and not options.profiler_in_distribution and options.profiler
# Map for custom evaluation logic
custom_evaluation_map = {
"JPH_PROFILE_ENABLED": custom_profiler_check
}
def generate(self):
for conan_option, cmake_option in conan_option_to_cmake_option_name.items():
tc.variables[cmake_option] = getattr(self.options, conan_option)
def package_info(self):
for conan_option, cmake_option in conan_option_to_cmake_option_name.items():
if getattr(self.options, conan_option):
self.cpp_info.defines.append(cmake_option)
for cmake_option, custom_func in custom_evaluation_map.items():
if custom_func(self.options):
self.cpp_info.defines.append(cmake_option)This would copy the settings for both the package and the consumer, let me know your thoughts on this, if you think it's alright I can go ahead with it. (There would be more code in the snippet but this is just specifying the framework |
|
Hello @cuppajoeman @AbrilRBS @noizex , |
At the moment I need some direction on how to handle the problem last specfied by @noizex when we have a clear direction on how to fix this then I think it can be finished pretty fast |
|
I'm willing to help as well. |
Honestly I think if you could try to get the attention of someone who is more experienced on the slack server to look over what we have that would be really useful, I've tried a few times but haven't had any luck |
|
JoltPhysics 5.2.0 released. |
|
Hey @franramirez688 lmk if you have any questions about this PR I can try to help out |
|
Hello @franramirez688 Hope you are well |
Signed-off-by: Uilian Ries <uilianries@gmail.com>
Signed-off-by: Uilian Ries <uilianries@gmail.com>
Signed-off-by: Uilian Ries <uilianries@gmail.com>
Signed-off-by: Uilian Ries <uilianries@gmail.com>
|
@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 <uilianries@gmail.com>
|
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. |
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