-
Notifications
You must be signed in to change notification settings - Fork 24
fix: cmake installation for yocto build #37
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
Conversation
Summary of ChangesHello @frogarian, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses an issue with CMake-based plugin installation, particularly within Yocto build environments. It introduces a configurable variable for the plugin installation directory, enabling external build systems to dictate the correct path. This change ensures that plugins are installed to the appropriate location when building with Yocto, while also providing robust default behavior for other build configurations. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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.
Code Review
This pull request effectively addresses the CMake installation process, particularly for Yocto builds, by introducing a flexible "INSTALL_PLUGINSDIR" cache variable. This allows external build systems to easily override the plugin installation path. The use of standard CMake variables like "CMAKE_INSTALL_LIBDIR" and "CMAKE_INSTALL_BINDIR" for default paths significantly improves the maintainability and portability of the build system. Additionally, centralizing the "platforminputcontexts" suffix in the "install" command enhances consistency.
src/CMakeLists.txt
Outdated
| set(INSTALL_PLUGINSDIR "" CACHE PATH "Installation directory for plugins") | ||
| if (NOT INSTALL_PLUGINSDIR) | ||
| if(${PROJECT_NAME} STREQUAL ${CMAKE_PROJECT_NAME}) | ||
| set(INSTALL_PLUGINSDIR ${CMAKE_INSTALL_LIBDIR}/plugins) |
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 like the fact QT_DIR variable is no longer used.
But this should not be changed because the thing fix: cmake installation for yocto build should be solved by using if (NOT INSTALL_PLUGINSDIR).
So this is an additional change to a thing that was working correctly before and was the desire of the developer.
I would recommend you create an issue(or in the PR conversation) explaining what you are trying to solve with this pr.
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.
Oh. It seems that something got mixed up. Actually I wanted to keep everything as is, except that if INSTALL_PLUGINSDIR is set, it should be used. Updated my code.
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.
Many thanks for your quick response. I was testing the CMake installation, and it does not work for me, even using the main branch.
In my opinion, this project should not depend on a Yocto recipe passed variable(as it should not depend on QT_DIR).
I think the general installation using CMake has to be fixed.
1705918 to
ea3577b
Compare
EddyTheCo
left a comment
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 is not clear to me what problem this PR is intended to solve.
This PR changes the installation location of the library: previously, the library was installed in the Qt installation path when it was the root CMake project, but with this change, it will no longer be.
This may affect how Qt is able to find this library and qml module.
By testing the project, the CMake installation is not working before and after this PR.
A proper issue and a more complete PR should be created.
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.
Now the PR uses only this INSTALL_PLUGINSDIR variable that can be set by passing -D to cmake and reduces duplicated text.
So I do not see why not to accept this PR.
INSTALL_PLUGINSDIRis set byqt6-cmake.bbclassin meta-qt6