-
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
Add bimg #15317
Add bimg #15317
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.
recipes/bimg/all/conanfile.py
Outdated
if not self.options.get_safe("fPIC", True): | ||
raise ConanInvalidConfiguration("This package does not support builds without fPIC.") |
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 say if it's not optional, you could remove it from options
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.
Doesn't the cci linter complain if fPIC is not an option? If not, I don't mind removing it, and will change bx later to do the same for consistency
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 will submit a warning message: WARN: [FPIC OPTION (KB-H006)] This recipe does not include an 'fPIC' option. Make sure you are using the right casing
But no error will raise
get(self, **self.conan_data["sources"][self.version], strip_root=True, | ||
destination=os.path.join(self.source_folder, self._bimg_folder)) | ||
#We need bx source too | ||
get(self, **self.dependencies["bx"].conan_data["sources"][self._bx_version[self.version]], strip_root=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.
Is it possible to patch instead and consume bx as package instead?
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 would be very difficult as the entire build system (genie project) relies on it being that way, and I'm not sure if I'm good enough with genie to modify it to do it that way instead. I could try to play around with doing that but I think it might be too much work (with even more in the future to keep it working) than it would be worth, considering it wouldn't actually affect projects that just use bgfx/bimg/bx and that grabbing bx source from the dependency package when building is easy and straightforward.
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 remember about the discussion using genie in another PR. If say it's very difficult, please, keep doing dual download, but add a comment in the recipe explaining it. So far, this build system is one of most exotics that we have in CCI.
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 fact that bimg needs bx source (and bgfx needs bimg and bx source) isn't really genie's fault, but it is how the genie projects for them are set up.
The initial reason was that bx and bimg were basically part of bgfx and they were only split out as separate projects when they started getting bigger, and it was decided to just need their source to make it easy for people to build bgfx (and bimg and bx) without dealing with managing dependencies, just by also cloning bimg and bx in the same place. This was, of course, back when dependency management didn't have many or any good options like conan.
Perhaps in the future this can be changed, maybe even by using conan, but at the moment there aren't really any plans for it upstream. I will add a comment explaining this for now, thanks for the feedback!
Remove unnecessary @ in requires Further clarify why we need bx source
This comment has been minimized.
This comment has been minimized.
Oh, there's a conan v2 pipeline going now? How exciting! |
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.
Just a small comment about reducing the test package. Looking good.
find_package(bimg REQUIRED CONFIG) | ||
|
||
add_executable(${PROJECT_NAME} test_package.cpp) | ||
set_target_properties(${PROJECT_NAME} PROPERTIES CXX_STANDARD 14 CXX_STANDARD_REQUIRED ON CXX_EXTENSIONS OFF) | ||
target_link_libraries(${PROJECT_NAME} bimg::bimg) |
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.
find_package(bimg REQUIRED CONFIG) | |
add_executable(${PROJECT_NAME} test_package.cpp) | |
set_target_properties(${PROJECT_NAME} PROPERTIES CXX_STANDARD 14 CXX_STANDARD_REQUIRED ON CXX_EXTENSIONS OFF) | |
target_link_libraries(${PROJECT_NAME} bimg::bimg) | |
add_subdirectory(${CMAKE_CURRENT_SOURCE_DIR}/../test_package/ | |
${CMAKE_CURRENT_BINARY_DIR}/test_package/) |
You can avoid duplication by loading ../test_package
@@ -0,0 +1,26 @@ | |||
#include <bx/bx.h> |
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.
Once you have re-used the test_package folder, it will use test_package/test_package.cpp instead of this duplicated file, so you can delete this file.
All this is part of a template that we have been using in CCI and may help you instead of writing for scratch for every new recipe: https://github.com/conan-io/conan-center-index/tree/master/docs/package_templates
Well, your build system is unique, so at least the test package you just need to copy/paste 😅
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.
Fair enough! Testing that unfortunately revealed that the test_v1_package doesn't actually build correctly on windows msvc locally (with or without that change) so I'll have to figure out why that is first 😅
Turns out, test_v1 wasn't working because I was using msvc rather than Visual Studio as the compiler in my profiles, and the old CMake generators don't correctly apply MT(d)/MD(d) from the newer msvc variant. 😞 |
Conan v1 pipeline ✔️All green in build 8 (
Conan v2 pipeline (informative, not required for merge) ❌
The v2 pipeline failed. Please, review the errors and note this will be required for pull requests to be merged in the near future. See details:Failure in build 8 (
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. |
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.
LGTM
Specify library name and version: bimg/cci.20230114
This is the bimg library, part of the direct dependencies of bgfx, and required for that to be added.
This requires bx source to build, and newer versions of bimg may not work with a significantly older version of bx nor the other way around - as such, I've decided to map bimg versions to the newest working bx version in the conanfile. It's likely bx, bimg and bgfx will get updated together on cci, but the update cadence of these is not synced - bx tends to be updated the least frequently, while bimg is updated more frequently, and bgfx the most frequently. As such, it's possibly multiple versions of bimg will use the same version of bx as well.
This package is very similar to the one for bx otherwise (#13866) and the notes from there apply here as well.