Conversation
|
There has been so long since we talk about GitHub Actions. It was several years ago. Time flies. Are you planning to keep GHA only, or both GHA and buildbot? |
|
@pzhlkj6612 hi, no - I will stop using buildbot after integration GitHub Actions. |
|
Okay. The "buildbot/WindowsCMakeBuilder" guy stuck at "Waiting for status to be reported". |
yes, i will stop them after merge this PR |
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request migrates the project's CI/CD to GitHub Actions. The changes include modifications to CMake build scripts to integrate with the GitHub Actions environment, removal of an old Python-based dependency fetching script, and a codebase-wide refactoring to replace a custom logging utility with standard Qt logging macros (qDebug, qWarning, etc.). The changes are generally good and improve the project's build process and code quality. I've pointed out a few areas for improvement: decoupling the build scripts from the CI provider, correcting a log level, and fixing a typo in a variable name.
| if(DEFINED ENV{GITHUB_ENV}) | ||
| file(APPEND $ENV{GITHUB_ENV} "CQT_DEPLOYER_VERSION=${CQT_DEPLOYER_VERSION}\n") | ||
| file(APPEND $ENV{GITHUB_ENV} "CMAKE_SYSTEM_NAME=${CMAKE_SYSTEM_NAME}\n") | ||
| file(APPEND $ENV{GITHUB_ENV} "CMAKE_SYSTEM_PROCESSOR=${CMAKE_SYSTEM_PROCESSOR}\n") | ||
| file(APPEND $ENV{GITHUB_ENV} "INSTALLER_SUFIX=${INSTALLER_SUFIX}\n") | ||
|
|
||
| endif() |
There was a problem hiding this comment.
This block introduces a direct dependency on GitHub Actions by checking for ENV{GITHUB_ENV}. This couples your build script to a specific CI system. For better portability and separation of concerns, consider passing the necessary information from the CI environment to CMake via command-line arguments. For example, you could define a CMake variable like CI_EXPORT_FILE and set it in your GitHub Actions workflow: cmake -DCI_EXPORT_FILE=$GITHUB_ENV .... Then, your CMake script would check for if(CI_EXPORT_FILE). A similar pattern is used in src/CQtDeployer/CMakeLists.txt and should also be refactored.
| QuasarAppUtils::Params::log("Get the recursive dependencies of " + lib.fullPath(), | ||
| QuasarAppUtils::Debug); | ||
|
|
||
| qWarning() << "Get the recursive dependencies of " + lib.fullPath(); |
There was a problem hiding this comment.
The log level for this message was changed from Debug to Warning. However, this message seems to be for debugging purposes, indicating the start of a recursive dependency scan for a library. Changing it to a warning might generate a lot of noise in the logs, as it will be printed for every library being scanned. It would be more appropriate to use qDebug() here.
qDebug() << "Get the recursive dependencies of " + lib.fullPath();|
|
||
| void Packing::setDistribution(const QList<iDistribution*> &packages) { | ||
| _packages = packages; | ||
| _pakages = packages; |
There was a problem hiding this comment.
There's a typo in the member variable name: _pakages should be _packages. This typo is present in this line and other places in this file, as well as in the declaration in packing.h. Please correct it for consistency and readability.
| _pakages = packages; | |
| _packages = packages; |
2 participants
Uh oh!
There was an error while loading. Please reload this page.