Repository navigation
Build Snap Package - #81
Conversation
|
""" WalkthroughThis update introduces Snapcraft packaging for the Linux version of the application, adds a Snapcraft configuration and desktop entry, and expands build scripts and workflows to support Snap package creation and artifact uploading. The CMake configuration and scripts are updated for flexible executable naming and improved compatibility, while Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer
participant CI as GitHub Actions CI
participant Snapcraft as Snapcraft
participant GHRelease as GitHub Release
Dev->>CI: Push code or tag (with Snapcraft config)
CI->>CI: Build Linux binaries (CMake, scripts)
CI->>CI: Package with CPack (7Z, ZIP, DEB, RPM, etc.)
CI->>Snapcraft: Build Snap package (snapcraft)
CI->>CI: Collect artifacts (including .snap)
CI->>GHRelease: Upload release assets (all packages, including Snap)
Possibly related PRs
Poem
""" Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. ✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Pull Request Overview
This pull request introduces support for building and packaging the SQLiteQueryAnalyzer as a Snap package and includes several build system and configuration enhancements.
- Added Snapcraft configuration and modifications to workflows for Snap package building and artifact publishing.
- Introduced cross-platform build scripts with updated behavior for Linux and macOS builds.
- Updated CMake configurations to support a custom executable name and fallback logic for older Qt versions.
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/project/snapcraft.yaml | New Snapcraft configuration file with package metadata and dependencies. |
| src/project/linux/usr/share/applications/sqlitequery.desktop | Desktop entry file for Linux integration. |
| src/project/build.sh | Bash build script updated for Snap packaging and installation steps. |
| src/project/build.ps1 | PowerShell build script streamlined by removing the Install switch and integrating snapcraft. |
| src/project/CMakeLists.txt | Updates to allow a custom executable name and to provide fallback logic for older Qt versions. |
| .github/workflows/release.yml | Added step to upload the Snap package as a release asset. |
| .github/workflows/linux.yml | Revised file path filters for workflow triggers. |
| .github/workflows/linux-template.yml | Updated version replacements and added a Snapcraft step in the workflow. |
Comments suppressed due to low confidence (1)
src/project/build.ps1:29
- [nitpick] The removal of the '$Install' switch in favor of directly invoking 'snapcraft' may affect local installation workflows. Verify that this change aligns with the intended behavior for Linux environments.
snapcraft
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (2)
src/project/CMakeLists.txt (2)
159-163:⚠️ Potential issueFix hard-coded target name for widget linking.
Thetarget_link_librariescall usesSQLiteQueryAnalyzerinstead of${APP_NAME}, breaking custom names. Update to:if(QT_VERSION_MAJOR GREATER 4) target_link_libraries(${APP_NAME} PRIVATE Qt::Widgets) endif()
165-168:⚠️ Potential issueUse
${APP_NAME}in install command.
Theinstall(TARGETS ...)stanza still referencesSQLiteQueryAnalyzerdirectly. It must use${APP_NAME}:install(TARGETS ${APP_NAME} BUNDLE DESTINATION . RUNTIME DESTINATION ${CMAKE_INSTALL_BINDIR} )
🧹 Nitpick comments (10)
src/project/build.ps1 (1)
35-39: Consider using relative path for macOS tooWhile the Linux section now uses a relative path for the install prefix, the macOS section still uses an absolute path. Consider using a relative path here as well for consistency.
- cmake -B build -DCMAKE_BUILD_TYPE=Release -DCMAKE_INSTALL_PREFIX=/tmp/sqlitequery + cmake -B build -DCMAKE_BUILD_TYPE=Release -DCMAKE_INSTALL_PREFIX=./macossrc/project/linux/usr/share/applications/sqlitequery.desktop (2)
8-8: Review desktop file categories.
TheQtcategory is not part of the Freedesktop standard and may be ignored by some desktop environments. Consider using recognized categories like:
Categories=Development;Database;
or addingUtilityif appropriate.
1-10: Enhance desktop integration with additional fields.
To improve user experience, consider adding:
•StartupNotify=true
• Placeholder%Ffor file opening, e.g.Exec=sqlitequery %F
•MimeType=application/x-sqlite3;
These can help with startup feedback and file associations.src/project/CMakeLists.txt (3)
15-20: Use a cached CMake option for EXECUTABLE_NAME.
Instead of checking only ifEXECUTABLE_NAMEis defined, declare it as a cache variable with a default to expose it in GUIs and CLI:set(EXECUTABLE_NAME "SQLiteQueryAnalyzer" CACHE STRING "Name of the application executable") set(APP_NAME ${EXECUTABLE_NAME})This documents the variable and makes overrides via
-DEXECUTABLE_NAMEmore discoverable.
170-175: Group deploy script generation with target.
Whileqt_generate_deploy_app_scriptcorrectly references${APP_NAME}, consider moving theinstall(SCRIPT ${deploy_script})call immediately after script generation for clarity and logical grouping.
177-187: Parameterize CPack settings with${APP_NAME}.
Hard-codingSQLiteQueryAnalyzerinCPACK_PACKAGE_NAME,CPACK_PACKAGE_INSTALL_DIRECTORY, etc., will desynchronize if the executable name changes. Use:set(CPACK_PACKAGE_NAME ${APP_NAME}) set(CPACK_PACKAGE_INSTALL_DIRECTORY ${APP_NAME})and similar for maintainer fields if needed.
src/project/snapcraft.yaml (1)
7-7: Remove trailing whitespace.
YAML lint flags trailing spaces on lines 7 and 10. Please delete the extra spaces at end of these lines to satisfy linting.Also applies to: 10-10
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 7-7: trailing spaces
(trailing-spaces)
src/project/build.sh (3)
45-55: DRY up repeated CPack invocations.
Multiplecpack -G <GEN>calls can be simplified:for GEN in 7Z ZIP TBZ2 TGZ TXZ TZ DEB RPM; do cpack -G "$GEN" --config ./build/CPackConfig.cmake done
57-102: Remove unreachable snap creation code.
Theif true; then … else … fialways takes the first branch, making the alternative snap-setup path dead code. Clean up or drive this logic with an explicit flag.
109-112: Quotesysctloutput to avoid word splitting.
ShellCheck (SC2046) warns about unquoted$(sysctl -n hw.ncpu). Change to:-cmake --build build --config Release --parallel $(sysctl -n hw.ncpu) +cmake --build build --config Release --parallel "$(sysctl -n hw.ncpu)"🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 111-111: Quote this to prevent word splitting.
(SC2046)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src/project/linux/usr/share/icons/hicolor/256x256/apps/sqlitequery.pngis excluded by!**/*.png
📒 Files selected for processing (9)
.github/workflows/linux-template.yml(3 hunks).github/workflows/linux.yml(1 hunks).github/workflows/release.yml(1 hunks).gitignore(1 hunks)src/project/CMakeLists.txt(2 hunks)src/project/build.ps1(3 hunks)src/project/build.sh(1 hunks)src/project/linux/usr/share/applications/sqlitequery.desktop(1 hunks)src/project/snapcraft.yaml(1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.35.1)
src/project/snapcraft.yaml
[error] 7-7: trailing spaces
(trailing-spaces)
[error] 10-10: trailing spaces
(trailing-spaces)
🪛 Shellcheck (0.10.0)
src/project/build.sh
[warning] 111-111: Quote this to prevent word splitting.
(SC2046)
🔇 Additional comments (15)
.gitignore (1)
49-63: LGTM! The gitignore additions are appropriateThe new entries correctly exclude the various Linux package formats and build artifacts that will be generated with the Snap packaging addition, including the Snap package file itself and all the supporting directories/files.
.github/workflows/release.yml (1)
134-142: Well-implemented upload step for Snap packageThe new step follows the same pattern as the existing upload steps and correctly references the Snap package path using the appropriate naming convention.
.github/workflows/linux.yml (1)
8-8: LGTM! Workflow trigger pattern updated correctlyThe wildcard pattern
linux*.ymlensures that changes to any Linux-related workflow file (including the new template) will trigger the workflow..github/workflows/linux-template.yml (5)
27-28: Good version synchronization for snapcraftEnsuring version consistency across all package definitions is important for proper release management.
40-42: Good change to use relative install pathUsing a relative path (
./linux) instead of an absolute path makes the build more portable and consistent across different environments.
45-55: LGTM! Well-structured packaging stepThe renamed packaging step provides comprehensive package format support, which improves distribution options for Linux users.
57-64: Snapcraft integration looks goodThe step properly installs the required snaps with appropriate permissions and sets the correct environment variable for host-based building.
71-72: Artifact path patterns correctly updatedThe path patterns now include both the traditional package formats and the new Snap package format.
src/project/build.ps1 (3)
3-3: Simplifying parameter list is goodRemoving the
$Installswitch simplifies the script interface while maintaining the necessary functionality.
16-18: Good updates to Linux build processThe changes to the Linux build section correctly use a relative path for installation and always perform the install step, which is required for packaging.
29-29: LGTM! Snapcraft integration addedAdding the
snapcraftcommand to the package section ensures Snap packages are built alongside other formats.src/project/linux/usr/share/applications/sqlitequery.desktop (1)
6-7: Validate Exec and Icon paths within snap environment.
The desktop entry usesExec=sqlitequery, which must resolve to the snap appsqlitequery(not the raw binary). Ensure yoursnapcraft.yaml’sapps.sqlitequery.commandaligns so that runningsqlitequeryin a terminal (or via the menu) invokes the correct binary inside the snap. Similarly, confirm that${SNAP}/usr/share/icons/hicolor/256x256/apps/sqlitequery.pngexists in the snap’s filesystem.src/project/CMakeLists.txt (1)
26-29: Compatibility check for qt_standard_project_setup is sound.
Guardingqt_standard_project_setup()withif(COMMAND ...)ensures older Qt versions don’t break. The placement before executable creation is appropriate.src/project/snapcraft.yaml (2)
25-28: Ensure desktop entry Exec matches snap app name.
Yourapps.sqlitequery.command: bin/SQLiteQueryAnalyzermust align with the.desktopfile’sExec=sqlitequery. Verify that installing the snap exposes thesqlitequerycommand (an alias for the binary), so the desktop entry launches the correct app.
31-35: Verifysource: linuxdirectory exists and is populated.
Thesnapcraft.yamlpart usessource: linux(local). Confirm thatsrc/project/linuxcontains the built artifacts (binary, desktop file, icons) when Snapcraft runs, or adjust the path accordingly.
Using 'if true;' to disable snap package creation is hard-coded and may be confusing for future maintenance. Consider using a feature flag or an environment-based check with a clear comment outlining that this is a temporary workaround. Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/project/build.sh (2)
32-37:⚠️ Potential issueFailing to build the Linux binaries: CMake commands are commented out
All core Linux build steps are disabled, so no artifacts will be produced. This is critical—uncomment or parameterize these lines to perform the actual build.Proposed diff:
- # cmake -B build -DCMAKE_BUILD_TYPE=Release -DCMAKE_INSTALL_PREFIX=./linux/ - # cmake --build build --config Release --parallel $(nproc) - # cmake --install build + cmake -B build -DCMAKE_BUILD_TYPE=Release -DCMAKE_INSTALL_PREFIX=./linux/ + cmake --build build --config Release --parallel "$(nproc)" + cmake --install buildMake sure
$(nproc)is quoted to prevent word splitting.
38-43:⚠️ Potential issueIncorrect directory copying causes nested
linuxfolder
Usingcp -rf ./linux /tmp/sqlitequerywill create/tmp/sqlitequery/linux/.... You likely intend to copy the contents of./linuxinto/tmp/sqlitequery.Suggested fix:
- mkdir -p ~/.local/bin - cp -rf ./linux /tmp/sqlitequery + mkdir -p ~/.local/bin + rm -rf /tmp/sqlitequery + mkdir -p /tmp/sqlitequery + cp -r ./linux/* /tmp/sqlitequery/ ln -sf /tmp/sqlitequery/bin/SQLiteQueryAnalyzer ~/.local/bin/sqlitequery echo "Installed to ~/.local/bin/sqlitequery"Also quote paths if they may contain spaces.
🧹 Nitpick comments (5)
src/project/build.sh (5)
5-5: Enhance script robustness with strict mode
Currently onlyset -eis enabled. To catch unset variables and pipeline failures, consider addingset -o nounset -o pipefail.set -e +set -o nounset -o pipefail
11-27: Improve POSIX-compliant argument parsing
Usingwhile (( "$#" )); dorelies on Bash arithmetic evaluation. For broader shell compatibility and clearer intent, consider:-while (( "$#" )); do +while [ "$#" -gt 0 ]; do case "$1" in --package) PACKAGE=true shift ;; --install) INSTALL=true shift ;; *) echo "Unknown argument: $1" echo "Usage: $0 [--package] [--install]" exit 1 ;; esac doneThis change avoids arithmetic context and makes the loop easier to read.
30-31: Normalize OS name to avoid case-sensitivity issues
OS=$(uname)may return different cases across platforms. Converting to lowercase ensures reliable comparisons:-OS=$(uname) +OS=$(uname | tr '[:upper:]' '[:lower:]')Then compare against
linuxanddarwin.
89-98: Duplicate “snap package creation is disabled” messaging in the else branch
Theelseblock asserts the feature is disabled, yet this is the branch for whenDISABLE_SNAPis not set. This is misleading.Either remove the disabling message or repurpose this branch to perform the real snap build.
111-112: Quote command substitution to prevent word splitting (SC2046)
Unquoted$(sysctl -n hw.ncpu)may split unexpectedly if output contains whitespace.- cmake --build build --config Release --parallel $(sysctl -n hw.ncpu) + cmake --build build --config Release --parallel "$(sysctl -n hw.ncpu)"🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 111-111: Quote this to prevent word splitting.
(SC2046)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/project/build.sh(1 hunks)
🧰 Additional context used
🪛 Shellcheck (0.10.0)
src/project/build.sh
[warning] 111-111: Quote this to prevent word splitting.
(SC2046)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: macos-intel / template
- GitHub Check: build / build
- GitHub Check: build / template
cp -rf ./linux /tmp/sqlitequery will nest the linux folder inside /tmp/sqlitequery
sing while (( "$#" )); do relies on Bash arithmetic evaluation. For broader shell compatibility and clearer intent, consider:
Unquoted $(sysctl -n hw.ncpu) may split unexpectedly if output contains whitespace.
The Qt category is not part of the Freedesktop standard and may be ignored by some desktop environments
To improve user experience, consider adding: • StartupNotify=true • Placeholder %F for file opening, e.g. Exec=sqlitequery %F • MimeType=application/x-sqlite3;
Multiple cpack -G <GEN> calls can be simplified
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/project/build.sh (2)
40-42: 🛠️ Refactor suggestionImprove installation directory handling.
The current approach might not work correctly if target directories don't exist or if there's an existing file at the symlink location.
- mkdir -p /tmp/sqlitequery - cp -rf ./linux/* /tmp/sqlitequery - ln -sf /tmp/sqlitequery/bin/SQLiteQueryAnalyzer ~/.local/bin/sqlitequery + rm -rf /tmp/sqlitequery + mkdir -p /tmp/sqlitequery + cp -rf ./linux/* /tmp/sqlitequery + # Remove existing symlink or file if it exists + rm -f ~/.local/bin/sqlitequery + ln -sf /tmp/sqlitequery/bin/SQLiteQueryAnalyzer ~/.local/bin/sqlitequery
52-91: 🛠️ Refactor suggestionClarify Snap package creation approach.
The script has two branches for Snap package handling but neither actually creates a Snap package, which is confusing. The messages themselves are clear, but the structure could be simplified.
Since both branches just provide instructions and don't build Snap packages, the environment variable check is unnecessary:
- # Check if snap package creation is disabled via the DISABLE_SNAP environment variable - if [ "${DISABLE_SNAP}" = "true" ]; then - echo "Snap package creation is disabled (DISABLE_SNAP=true)." - echo "To create snap packages on a host system, use one of these approaches:" - echo "" - echo "Option 1: Use the snapcraft Docker image (recommended):" - echo " docker run --rm -v \$(pwd):/build -w /build snapcore/snapcraft:stable snapcraft" - echo "" - echo "Option 2: On an Ubuntu host with snapd properly running:" - echo " sudo snap install snapcraft --classic" - echo " snapcraft" - echo "" - else - # For standard non-container environments - echo "Checking for snapcraft dependencies..." - - # Check for snapd first - if ! command -v snap &>/dev/null; then - echo "Installing snap..." - sudo apt update - sudo apt install -y snapd - # Ensure snapd socket is available - if ! systemctl is-active snapd.socket &>/dev/null; then - echo "Starting snapd.socket..." - sudo systemctl start snapd.socket - sleep 2 - fi - fi - - echo "Snap package creation is disabled in this version of the build script." - echo "To create snap packages on a host system, use one of these approaches:" - echo "" - echo "Option 1: Use the snapcraft Docker image (recommended):" - echo " docker run --rm -v \$(pwd):/build -w /build snapcore/snapcraft:stable snapcraft" - echo "" - echo "Option 2: On an Ubuntu host with snapd properly running:" - echo " sudo snap install snapcraft --classic" - echo " snapcraft" - echo "" - fi + echo "Snap package creation requires additional setup." + echo "To create snap packages on a host system, use one of these approaches:" + echo "" + echo "Option 1: Use the snapcraft Docker image (recommended):" + echo " docker run --rm -v \$(pwd):/build -w /build snapcore/snapcraft:stable snapcraft" + echo "" + echo "Option 2: On an Ubuntu host with snapd properly running:" + echo " sudo snap install snapcraft --classic" + echo " snapcraft" + echo ""Alternatively, if you want to enable actual Snap building in the future:
# Check if snap package creation is disabled via the DISABLE_SNAP environment variable if [ "${DISABLE_SNAP}" = "true" ]; then echo "Snap package creation is disabled (DISABLE_SNAP=true)." echo "To create snap packages on a host system, use one of these approaches:" echo "" echo "Option 1: Use the snapcraft Docker image (recommended):" echo " docker run --rm -v \$(pwd):/build -w /build snapcore/snapcraft:stable snapcraft" echo "" echo "Option 2: On an Ubuntu host with snapd properly running:" echo " sudo snap install snapcraft --classic" echo " snapcraft" echo "" else # For standard non-container environments echo "Checking for snapcraft dependencies..." # Check for snapd first if ! command -v snap &>/dev/null; then echo "Installing snap..." sudo apt update sudo apt install -y snapd # Ensure snapd socket is available if ! systemctl is-active snapd.socket &>/dev/null; then echo "Starting snapd.socket..." sudo systemctl start snapd.socket sleep 2 fi fi - echo "Snap package creation is disabled in this version of the build script." - echo "To create snap packages on a host system, use one of these approaches:" - echo "" - echo "Option 1: Use the snapcraft Docker image (recommended):" - echo " docker run --rm -v \$(pwd):/build -w /build snapcore/snapcraft:stable snapcraft" - echo "" - echo "Option 2: On an Ubuntu host with snapd properly running:" - echo " sudo snap install snapcraft --classic" - echo " snapcraft" - echo "" + # Install snapcraft if not available + if ! command -v snapcraft &>/dev/null; then + echo "Installing snapcraft..." + sudo snap install snapcraft --classic + fi + + echo "Building snap package..." + snapcraft fi
🧹 Nitpick comments (4)
src/project/build.ps1 (2)
33-40: Inconsistent conditional structure for macOS.The macOS section uses a different conditional pattern than Windows and Linux sections.
-if ($IsMacOS -And $Package) { +if ($IsMacOS) { cmake -B build -DCMAKE_BUILD_TYPE=Release -DCMAKE_INSTALL_PREFIX=/tmp/sqlitequery cmake --build build --config Release --parallel 32 if ($Package) { macdeployqt build/SQLiteQueryAnalyzer.app -dmg -appstore-compliant } }
36-36: Standardize parallel build configuration.Consider using a dynamic processor count for macOS as you did for Linux.
- cmake --build build --config Release --parallel 32 + cmake --build build --config Release --parallel $(sysctl -n hw.ncpu)src/project/build.sh (2)
35-35: Fix potential word splitting in parallel build command.The
$(nproc)command output should be quoted to prevent word splitting issues.- cmake --build build --config Release --parallel $(nproc) + cmake --build build --config Release --parallel "$(nproc)"🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 35-35: Quote this to prevent word splitting.
(SC2046)
32-107: Consider adding Windows support with cross-platform checks.The script detects Linux and macOS but doesn't handle Windows. While this may be intentional for a bash script, consider adding a check for Windows with MSYS2/Git Bash environments.
# Detect OS OS=$(uname) + +# Check for Windows with MSYS2/Git Bash +if [[ "$OS" == MINGW* ]] || [[ "$OS" == CYGWIN* ]]; then + echo "This script is not designed for Windows. Please use build.ps1 instead." + exit 1 +fi🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 35-35: Quote this to prevent word splitting.
(SC2046)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/project/CMakeLists.txt(2 hunks)src/project/build.ps1(3 hunks)src/project/build.sh(1 hunks)src/project/linux/usr/share/applications/sqlitequery.desktop(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/project/linux/usr/share/applications/sqlitequery.desktop
- src/project/CMakeLists.txt
🧰 Additional context used
🪛 Shellcheck (0.10.0)
src/project/build.sh
[warning] 35-35: Quote this to prevent word splitting.
(SC2046)
🔇 Additional comments (5)
src/project/build.ps1 (2)
3-3: Parameter simplification looks good.Removing the
$Installswitch parameter and keeping only$Packagestreamlines the script interface.
16-18: Improvements to Linux build configuration.The changes to use a relative install path and dynamic parallel build are good improvements:
- Using
./linux/as the install prefix aligns with the new cross-platform build approach- Using
$(nproc)for parallelism ensures optimal performance across different Linux systems- Unconditionally running the install step simplifies the workflow
src/project/build.sh (3)
1-31: Well-structured script initialization.The script has a clean organization with proper error handling (
set -e), clear argument parsing, and OS detection. The usage instructions are helpful for users.
48-50: Efficient package generation with loop.Good use of a loop to generate multiple package formats efficiently.
100-100: Consistent quoting of command substitution.Good use of quotes around the command substitution for macOS CPU count.
The script directly calls snapcraft without checking if it's installed, which could cause build failures.
The current approach might not work correctly if target directories don't exist or if there's an existing file at the symlink location.
The script detects Linux and macOS but doesn't handle Windows. While this may be intentional for a bash script, consider adding a check for Windows with MSYS2/Git Bash environments.
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/project/build.ps1 (1)
30-35: Well-implemented snapcraft availability check.This is a good implementation of the snapcraft verification. The script now properly checks if snapcraft is available before attempting to run it and provides helpful messages for installation if it's missing.
src/project/build.sh (1)
60-102:⚠️ Potential issueSnap package creation is disabled in the script.
While the script has good handling for the
DISABLE_SNAPenvironment variable, both branches explicitly disable actual Snap package creation. This contradicts the PR objective of adding Snap packaging support.The script should actually invoke
snapcraftin the else branch (line 89-98). Consider replacing:- echo "Snap package creation is disabled in this version of the build script." - echo "To create snap packages on a host system, use one of these approaches:" - echo "" - echo "Option 1: Use the snapcraft Docker image (recommended):" - echo " docker run --rm -v \$(pwd):/build -w /build snapcore/snapcraft:stable snapcraft" - echo "" - echo "Option 2: On an Ubuntu host with snapd properly running:" - echo " sudo snap install snapcraft --classic" - echo " snapcraft" - echo "" + echo "Installing snapcraft and creating snap package..." + sudo snap install snapcraft --classic + snapcraft --destructive-modeThis change would align the script with the PowerShell version and the PR objectives.
🧹 Nitpick comments (1)
src/project/build.sh (1)
41-41: Fix potential word splitting issue.The command substitution in the parallel flag should be quoted to prevent word splitting.
- cmake --build build --config Release --parallel $(nproc) + cmake --build build --config Release --parallel "$(nproc)"🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 41-41: Quote this to prevent word splitting.
(SC2046)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/project/build.ps1(3 hunks)src/project/build.sh(1 hunks)
🧰 Additional context used
🪛 Shellcheck (0.10.0)
src/project/build.sh
[warning] 41-41: Quote this to prevent word splitting.
(SC2046)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: macos-intel / template
- GitHub Check: build / template
- GitHub Check: build / build
🔇 Additional comments (4)
src/project/build.ps1 (1)
16-18: Good improvements to the Linux build configuration!The changes to use a relative install prefix (
./linux/) and dynamic CPU core detection ($(nproc)) make the build process more flexible and efficient. These modifications properly align with the Snap packaging requirements.src/project/build.sh (3)
4-5: Good error handling setup.Using
set -eensures the script will exit immediately if any command fails, which is a good practice for build scripts.
48-48: Installation directory copying looks correct.The script now properly creates the destination directory before copying files, ensuring the files are copied correctly into
/tmp/sqlitequery.
107-108: Good use of dynamic CPU detection on macOS.Using
$(sysctl -n hw.ncpu)to detect available CPU cores for parallelism is an excellent practice for optimizing build performance on macOS.
This pull request introduces support for building and packaging the SQLiteQueryAnalyzer as a Snap package for Linux, along with several related improvements to the build system and configuration files. The changes include updates to workflows, build scripts, and CMake configurations to ensure compatibility with Snapcraft and older Qt versions.
Snap Package Support and Workflow Enhancements:
src/project/snapcraft.yamlto define the Snap package metadata, dependencies, and build instructions..github/workflows/linux-template.ymlto include Snapcraft installation, build steps, and artifact publishing for Snap packages. [1] [2].github/workflows/release.ymlto upload Snap packages as release assets.Build System Improvements:
build.shas a cross-platform build script with support for Snap packaging and installation.build.ps1to align Linux build steps with Snapcraft requirements and removed theInstallswitch. [1] [2]CMake Configuration Updates:
EXECUTABLE_NAMEparameter inCMakeLists.txt.CMakeLists.txtto handle older Qt versions by replacingqt_add_executableandqt_standard_project_setupwith manual configurations.CMakeLists.txtto use the newAPP_NAMEvariable.Additional Improvements:
sqlitequery.desktop) for the Snap package to integrate with Linux desktop environments..github/workflows/linux.ymlto match multiple Linux workflow files.Summary by CodeRabbit
New Features
Chores
Documentation