Closed ahmedyarub closed 3 years ago
OlivierLDff
commented
3 years ago Ok this is lot to review Can I extract from this PR the typo things to merge that first? You should we move headers? What does PUBLIC_HEADER exactly is for? Couldn't understand. I think best move for spdlog is to completely remove it and use qWarning instead. I also think that QOlm should be move as a private dependency because all the type of Qaterial are only meant to be used from qml.
ahmedyarub
commented
3 years ago Ok this is lot to review Can I extract from this PR the typo things to merge that first? You should we move headers? What does PUBLIC_HEADER exactly is for? Couldn't understand. I think best move for spdlog is to completely remove it and use qWarning instead. I also think that QOlm should be move as a private dependency because all the type of Qaterial are only meant to be used from qml.
1- Regarding the typos: please do, yes. I can do that for you if you don't have time. 2/3- Private headers are used only during the compilation of Qaterial itself, they are not referenced by other apps consuming Qaterial (such as Qaterial Gallery). While public ones are the only ones that need to be packaged together with the compiled binaries since other apps would reference them. I used separate folders to correctly do target_include_directories(${QATERIAL_TARGET} PUBLIC....) and target_include_directories(${QATERIAL_TARGET} PRIVATE....) where the former denotes the ones to be installed and the second one includes only the headers required during compilation of Qaterial. 4- I'm OK with refactoring the code to completely remove the dependency on spdlog. Are you sure that it's not required for some feature? 5- Regarding QOlm: that actually makes sense but remember that for static library compilation I will still have to install QOlm together with Qaterial so making it private here wouldn't make that much difference.
OlivierLDff
commented
3 years ago Ok so I started to merge and simplify stuff in v1.4.5 branch.
Utils.hpp, that is itself included by Qaterial.hpp. C++ public API of Qaterial is only to register/load ressources. So public headers of Qaterial don't have any dependency even to Qt. Version singleton is know private header, so I added version getter in Utils.hppNow I will merge the install code (same I did with QOlm, I will make it optional with QATERIAL_ENABLE_INSTALL).
Edit: to test that install is going successful, best is just to test if find_package is working right?
OlivierLDff
commented
3 years ago So I merged everything in https://github.com/OlivierLDff/Qaterial/releases/tag/v1.4.5 Feel free to reopen if something is missing
I made this PR to speedup compilation and development of projects using Qaterial. I noticed that including Qaterial into my own project increased both the configuration and building time significantly by parsing Qaterial's CMake scripts and running Auto MOC. In addition to that, this enabled me to cache the Qaterial builds in my Github workflows, saving minutes from each pipeline. I rearranged the headers to expose only the ones consumed by external projects, and also updated QOlm to have installation support (https://github.com/OlivierLDff/QOlm/pull/7). There are also two changes made here: Added the option to build spdlog or retrieve it using find_package for the same reasons above. And also fixed a typo in the name of one of the components.
Note: I had to disable the configuration of installation scripts when spdlog's source code is included instead of being imported using find_package() because spdlog does not have export sets (using export() command) which gives an error when trying to include it in the installation scripts.