-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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 rmlui/3.3 #5122
Add rmlui/3.3 #5122
Conversation
Sorry, the build is only launched for Early Access Program users. You can request access writing in this issue. |
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 notice there new hooks that were not followed such as fPIC, please update your looks locally and should be easy to fix =)
Co-authored-by: Chris Mc <[email protected]>
Sorry, the build is only launched for Early Access Program users. You can request access writing in this issue. |
Co-authored-by: Chris Mc <[email protected]>
Sorry, the build is only launched for Early Access Program users. You can request access writing in this issue. |
Sorry, the build is only launched for Early Access Program users. You can request access writing in this issue. |
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! 🚀
Sorry, the build is only launched for Early Access Program users. You can request access writing in this issue. |
Thanks for the thorough review! |
Failure in build 6 (
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. |
Our usual safe option is |
Isn't there a difference between a feature and an extra dependency? |
Co-authored-by: Chris Mc <[email protected]>
Failure in build 7 (
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. |
I like this approach! |
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. |
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. |
Failure in build 10 (
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. |
All green in build 11 (
|
@madebr @prince-chrismc I think the recipe has come out looking pretty slick, so thanks for all the suggestions! :-) |
self._cmake.definitions["BUILD_LUA_BINDINGS"] = self.options.with_lua_bindings | ||
self._cmake.definitions["BUILD_SAMPLES"] = False | ||
self._cmake.definitions["DISABLE_RTTI_AND_EXCEPTIONS"] = not self.options.enable_rtti_and_exceptions | ||
self._cmake.definitions["ENABLE_PRECOMPILED_HEADERS"] = 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.
The only thing I'm left wondering about is whether it is worth to require cmake for building with precompiled headers.
The only benefit precompiled headers have is potential faster compile times. The resulting binary should not differ.
Is it worth it for rmlui?
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.
Well.. the answer to that would depend largely on the build environment. Therefore, I checked the build logs to see what happens and noted that each build seems to start off with a clean slate as far as cached packages are concerned.
With this in mind, I made some measurements locally using MSVC, to be able to provide a rough estimate as to the improvement in build time. I used the following commands to build packages and measured until after the test had concluded:
conan remove cmake -f
conan create . 3.3@te/st
attempt | with pch | without pch |
---|---|---|
1 | 1:59 | 2:45 |
2 | 2:04 | 2:31 |
Based on the above, I'd assume that using precompiled headers would save the build system serving conan-center-index valuable time in case of other compilers as well. Since improvement in build time should be considered a positive outcome in my opinion, I would conclude that it's indeed worth it - unless I'm missing something that would also play a factor in the current context.
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 for the benchmark!.
A >25% performance increase is indeed impressive.
Though, it should be noted that this cost is only paid once (on the cci servers) or when somebody is building the package themselve.
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 should be noted that this cost is only paid once
It's fairly rare that consumers actually build the library... even more rare they build twice... (whole point of Conan is it manages the binaries for you)
For that reason we've avoided these types of things because they were complicated to manage for CCI ... but it's only two lines.
We'll see if the Conan team has any objectives.
* Implement the initial version of the recipe * Update license to MIT instead of direct link Co-authored-by: Chris Mc <[email protected]> * Update the test recipe Co-authored-by: Chris Mc <[email protected]> * Make improvements based on review comments * Remove the option to build samples * Set the C++ standard of the test package Co-authored-by: Chris Mc <[email protected]> * Rework the options * Make improvements based on the reviews * Make a certain version CMake a build requirement * Check compiler-eligibility the right way Co-authored-by: ZombieRaccoon <[email protected]> Co-authored-by: Chris Mc <[email protected]>
Specify library name and version: rmlui/3.3
Any comments on the recipe are much appreciated!
Excerpt from the library's readme.md:
"RmlUi is the C++ user interface package based on the HTML and CSS standards, designed as a complete solution for any project's interface needs. It is a fork of the libRocket project, introducing new features, bug fixes, and performance improvements."
More information on the library itself can be found here: https://github.com/mikke89/RmlUi
conan-center hook activated.