-
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
bgfx: add new version and consolidated recipe #24423
base: master
Are you sure you want to change the base?
Conversation
This comment has been minimized.
This comment has been minimized.
This needs osx >= 13, how do we limit it to that? Looks like it's trying to use 11.3 and the cci profiles do not provide os.version or os.sdk_version for osx, it seems |
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.
Recommending a new recipe folder name + updating the bx package to a different version so it's in sync with bgfx.
Move old recipe to cci2023 Use matching bx commit Add cppstd handling for conan 1 to toolchain generators Add apple sdk checks Change warning for compiler check
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
On conan 1, it still doesn't seem to use C++17 even with setting tc.cppstd = "-std=c++17" manually. I'm guessing tc.generate() overwrites it from self.settings? |
tc.generate() | ||
else: | ||
tc = AutotoolsToolchain(self) | ||
if not self.settings.get_safe("compiler.cppstd"): | ||
tc.cppstd = "-std=c++17" |
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.
Filed as conan-io/conan#16533
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.
Try with-macos=11 for macos [11, 13)
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.
@AbrilRBS when you have the time, it would be great if we could figure out how to proceed on this. |
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.
Looks good. I double checked the commits and hashes.
Hey @AbrilRBS, sorry to bother you but is there any way you're aware of to escalate this topic for assessment? Or any workarounds? |
# Conflicts: # recipes/bgfx/all/conanfile.py
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Conan v1 pipeline ❌Warning Conan Center will stop receiving updates for Conan 1.x packages soon - please see announcement. Failure in build 9 (
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 9 (
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. |
Summary
Changes to recipe: bgfx
Motivation
This adds a consolidated recipe for bgfx going forward. This is versioned using bgfx' versioning scheme (https://github.com/bkaradzic/bgfx/blob/93961afcfd45bdcbdb91bddfb133a621d340f136/src/bgfx.cpp#L3566), and integrates bx and bimg into this package. This was done after discussions about whether having bx and bimg separate is valuable, and how due to their interdependence, need for dependency sources, and reliance on specific versions, having them separate makes updates on CCI much slower and more cumbersome.
Details
The recipe is still based on the previous bgfx recipe, but with quite a few changes and additions.
It adds all things required for bimg and bx to be integrated into this recipe, such as defines, libs, and their headers.
It also adds an option for rtti, due to the fact that bgfx' (and bx', and bimg's) build scripts are hardcoded to build with rtti disabled. Packages on CCI are rtti enabled by default, and mixing these can cause linker issues, especially on linux in my experience.
There is also improved support for mingw and android, changes to make other things more consistent, and so on.
The new recipe has been locally tested on windows 11 msvc194 and mingw, linux gcc, and android.
Some questions I was pondering:
Would there be value in adding an option for overriding bx' CRT detection in order for this to build with something like musl?
Would there be value in exposing options for opengl version etc. and other compile-time options that bgfx has?