-
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
raylib: add some common build settings to options #24585
base: master
Are you sure you want to change the base?
Conversation
This comment has been minimized.
This comment has been minimized.
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 @Julianiolo for taking the time to add the options :)
Co-authored-by: Abril Rincón Blanco <git@rinconblanco.es>
This comment has been minimized.
This comment has been minimized.
As this configuration is not exposed as project option, I would suggest using Conan configuration to pass any custom definition to the compiler instead: https://docs.conan.io/2/reference/config_files/global_conf.html You can add it to your profile, or use via command line directly:
This configuration will pass As it may affect the package ID, in case changing the result of the library behavior, you still can inject that configuration as part of the package ID as well:
So, in summary |
@uilianries I think they are intended as project options, raylib just manages them via this config.h file, not via some build script. (as It also has build scripts for like 5+ different build systems) The intention is to be able to set these options from a consuming recipe, would that even be possible with the configuration? |
@Julianiolo I'm reading raylib cmake files and indeed those options are exposed: https://github.com/raysan5/raylib/blob/5.0/CMakeOptions.txt#L31. Why passing via
Configurations work for both cases, building and consuming. |
Oh WOW, how did I not see that lol, I was looking at the wrong CMakeLists.txt.... |
This comment has been minimized.
This comment has been minimized.
@Julianiolo That could work, but need to test first. Thank for spotting it. |
recipes/raylib/all/conanfile.py
Outdated
true_false = lambda x: True if x else False | ||
tc.variables["SUPPORT_MODULE_RSHAPES"] = true_false(self.options.module_rshapes) |
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.
true_false = lambda x: True if x else False | |
tc.variables["SUPPORT_MODULE_RSHAPES"] = true_false(self.options.module_rshapes) | |
tc.variables["SUPPORT_MODULE_RSHAPES"] = self.options.module_rshapes |
The OptionValue is converted to boolean already, you should not need another helper function.
recipes/raylib/all/conanfile.py
Outdated
} | ||
default_options = { | ||
"shared": False, | ||
"fPIC": True, | ||
"opengl_version": None, | ||
|
||
"module_rshapes": 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.
I would suggest adding only option that you really need for now, otherwise, it will increase the recipe maintenance. We can not validate each combination, which means there is a risk breaking things depending the combination.
I also would request a build log, if possible, using the options that you are adding, I mean, using the non-default option value, to make sure it's not broken. I asking it because we found recipes with custom options that are not working and the CI really does not check it (only shared, fPIC and header_only are checked), so any user opens an issue months later reporting about that option is broken and recipe/upstream is bugged.
Co-authored-by: Uilian Ries <uilianries@gmail.com>
@uilianries I just saw this: raysan5/raylib@307c998 So I guess maybe the patching approach is actually more future safe? On second thought, why does it parse the config.h to then produce cmake options, when it could just include the config.h in the code?? |
This comment has been minimized.
This comment has been minimized.
@Julianiolo because we avoid patching anything as we will become the maintainers of that patch. The project offers transparent options via CMake, and it reflects to the Conan generator CMaketoolchain, that's is well prepared and integrated to work with the project, and we have been using it for any other project. So, using CMake will consume less maintenance from our side. |
@uilianries No, with that commit, it does not support options via cmake anymore (or am I reading this wrong?) I'm going to investigate this... |
Ok, turns out I was reading that completely wrong, lol |
This comment has been minimized.
This comment has been minimized.
So now I removed some options, and added a @uilianries Do you mean by build logs just the output from |
Build log
|
This comment has been minimized.
This comment has been minimized.
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.
@Julianiolo Thank you for updating your PR.
There are still points that need few changes:
- The raylib project does not use customize build by default, neither other package managers that I visited are using it by default, so I don't recommend doing it by default as will affect every user.
- In case needing to extra cmake definitions, you still can use Conan config, e.g:
conan install ... -c -c tools.cmake.cmaketoolchain:extra_variables='{"SUPPORT_CAMERA_SYSTEM": "ON", "SUPPORT_GESTURES_SYSTEM": "ON"}'
The same can be part of your profile. You can obtain more information in the CMakeToolchain and Config docs page.
Still, it will not affect you package ID directly, so you can enforce it by using the tools.info.package_id:confs
:
-c tools.info.package_id:confs='["tools.cmake.cmaketoolchain:extra_variables"]'
This will tell Conan to use extra_variables as part of the Package ID.
- The options
module_raudio
,events_waiting
, andcustom_frame_control
are directly affected bycustomize_build
, so I would suggest removing them in configure, as you can not change them in case customize_build is False:
def configure(self):
if not self.customize_build:
del module_raudio
del events_waiting
del custom_frame_control
- You do not need to parse to "ON"/"OFF" those options values, the boolean values work perfectly with CMake, we have being using it since many years and is compatible with both CMake 2.8 and +3.x
recipes/raylib/all/conanfile.py
Outdated
|
||
tc.variables["CUSTOMIZE_BUILD"] = "ON" | ||
on_off = lambda x: "ON" if x else "OFF" | ||
tc.variables["SUPPORT_MODULE_RAUDIO"] = on_off(self.options.module_raudio) | ||
|
||
# this makes it include the headers rcamera.h, rgesture.h and rprand.h | ||
tc.variables["SUPPORT_CAMERA_SYSTEM"] = "ON" | ||
tc.variables["SUPPORT_GESTURES_SYSTEM"] = "ON" | ||
tc.variables["SUPPORT_RPRAND_GENERATOR"] = "ON" | ||
|
||
tc.variables["SUPPORT_EVENTS_WAITING"] = on_off(self.options.events_waiting) | ||
tc.variables["SUPPORT_CUSTOM_FRAME_CONTROL"] = on_off(self.options.custom_frame_control) | ||
|
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.
tc.variables["CUSTOMIZE_BUILD"] = "ON" | |
on_off = lambda x: "ON" if x else "OFF" | |
tc.variables["SUPPORT_MODULE_RAUDIO"] = on_off(self.options.module_raudio) | |
# this makes it include the headers rcamera.h, rgesture.h and rprand.h | |
tc.variables["SUPPORT_CAMERA_SYSTEM"] = "ON" | |
tc.variables["SUPPORT_GESTURES_SYSTEM"] = "ON" | |
tc.variables["SUPPORT_RPRAND_GENERATOR"] = "ON" | |
tc.variables["SUPPORT_EVENTS_WAITING"] = on_off(self.options.events_waiting) | |
tc.variables["SUPPORT_CUSTOM_FRAME_CONTROL"] = on_off(self.options.custom_frame_control) | |
tc.variables["CUSTOMIZE_BUILD"] = self.options.customize_build | |
if self.options.customize_build: | |
tc.variables["SUPPORT_MODULE_RAUDIO"] = self.options.get_safe("module_raudio") | |
tc.variables["SUPPORT_EVENTS_WAITING"] = self.options.get_safe("events_waiting") | |
tc.variables["SUPPORT_CUSTOM_FRAME_CONTROL"] = self.options.get_safe("custom_frame_control") |
I would ask to remove this forced customization and simplify it instead:
- The upstream goes to the opposite way by default: https://github.com/raysan5/raylib/blob/5.0/CMakeOptions.txt#L11. The
CUSTOMIZE_BUILD
disabled, same for camera, gestures and rprand - Not even others package managers are customizing it, real example:
- AUR: https://gitlab.archlinux.org/archlinux/packaging/packages/raylib/-/blob/main/PKGBUILD?ref_type=heads
- Fedora: https://src.fedoraproject.org/rpms/raylib/blob/rawhide/f/raylib.spec
- Homebrew: https://github.com/Homebrew/homebrew-core/blob/79b0412c280d8a1283a2f38f444cad436261e1c9/Formula/r/raylib.rb
@uilianries huge thanks for taking the time to review this so thoroughly :) I see you removed the
This was designed as a fix to #24472, but I guess it does not work without the customized build. So maybe doing it like the PR suggests is better? Btw, does turning on customize build by itself even do anything? Build log======== Exporting recipe to the cache ======== ======== Input profiles ======== Profile build: ======== Computing dependency graph ======== ======== Computing necessary packages ======== ======== Installing packages ======== -------- Installing package raylib/5.0 (3 of 3) -------- Update the VERSION argument value or use a ... suffix to tell -- Using Conan toolchain: C:/Users/-/.conan2/p/b/rayliee81208d77cdb/b/build/generators/conan_toolchain.cmake raylib/5.0: Running CMake.build() 1>Checking Build System raylib/5.0: Package 'ddd4486cb5134320eb1ec58b8e53a4272a2d2fad' built raylib/5.0: package(): Packaged 3 '.h' files: raylib.h, raymath.h, rlgl.h ======== Launching test_package ======== ======== Computing dependency graph ======== ======== Computing necessary packages ======== ======== Installing packages ======== ======== Testing the package ======== ======== Testing the package: Building ======== Update the VERSION argument value or use a ... suffix to tell -- Using Conan toolchain: C:/Users/--/cpp/libs/conan-center-index/recipes/raylib/all/test_package/build/msvc-193-x86_64-20-release/generators/conan_toolchain.cmake raylib/5.0 (test package): Running CMake.build() 1>Checking Build System ======== Testing the package: Executing test ======== |
@Julianiolo Thank you for pointing that detail! I'll discuss with Abril about it!
Yes, the upstream uses |
@uilianries my question is if enabling customize build actually changes anything in the result. Shouldn't it just produce the exact same thing if no other options are changed? |
@Julianiolo No, https://github.com/raysan5/raylib/blob/5.0/CMakeOptions.txt#L38
The option More information: https://cmake.org/cmake/help/latest/module/CMakeDependentOption.html |
@uilianries But shouldn't it still be ON if customize build is OFF, due to |
@Julianiolo Correct, but you may consider https://github.com/raysan5/raylib/blob/5.0/cmake/CompileDefinitions.cmake#L12 Without After talking with @AbrilRBS, we considered exposing those 3 options Speaking about the PR #24472, that PR is copying those extra headers, but the upstream does not copy and will not (It was asked directly to the upstream). So, do you need those headers as well? In case positive, will need to copy them according to the active option. |
@uilianries I thought that enabling |
@Julianiolo Sorry, I did not understand what you mean. What I can say: Raylib will not copy camera headers even SUPPORT_CAMERA_SYSTEM is enabled, you need to copy them manually as #24472 |
Huh so I thought that enabling for example I currently don't need those headers. But maybe we should indeed then use a solution like outlined in #24472. |
So this is my current state, everything seems to work except that the copy in the test conanfile.py fails (the one that is supposed to copy the rcamera.h). See:
I checked and the rcamera.h and others are properly copied into the res folder of the packege so that should not be the issue. I guess the |
This comment has been minimized.
This comment has been minimized.
@Julianiolo Correct. I know @AbrilRBS asked to the upstream about, but was rejected the idea of copying those headers natively. In summary, we need to copy manually via
Could you please clarify? Do you need that feature (e.g. camera) enabled in the library, but not exposed by headers? Asking because if there is no usage for those headers, we could skip copying them as well. |
@uilianries Well for my current use case they aren't needed (neither being build into the lib nor being accessible to include), but as there is already a PR for it, and it is probably a somewhat common need, we should probably at least make the headers accessible. The building into the lib is kinda not nescessary i guess, as you can just do the implementation in your own c/cpp file. |
@Julianiolo thank you for your quick answer! I'll check your failed test package. 😄 |
@uilianries I found the issue, now the remaining question is, how do you add your custom copy location to the include directories? |
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 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. |
@Julianiolo You need to change the |
@uilianries does that also work for the test package? I tried it and it does not seem to work. |
@Julianiolo What do you mean exactly? The test package should receive the include dirs from Conan generators, like CMakeDeps. In the test pacakge, you should not need to configure dependencies include dirs, only consume them. |
@uilianries well we copy the headers in the test package ( the |
@uilianries any idea on this? |
Hello @Julianiolo ! I'm on vacation 🌴, please, ping |
@uilianries ah sorry then, have a nice vacation :) |
Summary
Changes to recipe: raylib/*
Motivation
raylib defines a lot of build settings in
config.h
. I added some of the most common/important ones to the recipe optionsDetails
We just patch the
config.h
file to match the options. This seems to be the easiest way to do this, and it makes theconfig.h
file mirror the current configuration (although that file is currently not exported; I guess this is relevant to that: #24472).