fix #1557: no QR code #1609
fix #1557: no QR code #1609
Conversation
|
""" WalkthroughA new package group variable Changes
Poem
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure 🪧 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.
Actionable comments posted: 0
🧹 Nitpick comments (1)
depends/packages/packages.mk (1)
13-13: Add QRencode package group—verify inclusion in build.The new
qrencode_packages=qrencodeentry follows existing naming patterns. Please confirm that this group is actually pulled into your default build or the appropriate platform-specific targets. If you intend QRencode as a default dependency, consider updating the mainpackageslist:-packages:=boost openssl libevent gmp zlib backtrace tor bls-dash +packages:=boost openssl libevent gmp zlib backtrace tor bls-dash qrencode
cd799cb to
aa0820c
Compare
aa0820c to
a1bfbd0
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
depends/packages/qrencode.mk (1)
12-12: Warning suppression for compilationCompiler flags have been added to suppress specific warnings that might otherwise halt the build. While this allows the compilation to proceed, it would be ideal to address the root causes of these warnings in a future update if possible.
Consider investigating and fixing the underlying causes of these warnings in the future rather than suppressing them, especially the implicit function declarations which can lead to runtime issues on different architectures.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
depends/packages/packages.mk(1 hunks)depends/packages/qrencode.mk(2 hunks)depends/patches/qrencode/cmake_fixups.patch(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- depends/patches/qrencode/cmake_fixups.patch
🚧 Files skipped from review as they are similar to previous changes (1)
- depends/packages/packages.mk
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Build for Windows (CMake)
- GitHub Check: Build for Linux (CMake)
- GitHub Check: Build for macOS (CMake)
- GitHub Check: Create Source Distribution
🔇 Additional comments (7)
depends/packages/qrencode.mk (7)
3-5: Source URL and package identification updated correctlyThe changes update the download path to the official source, update the file name pattern, and provide the correct SHA256 hash for verification. These changes are essential for the package to be downloaded and verified correctly.
9-11: CMake build configuration properly set upThe configuration has been migrated from autoconf to CMake with appropriate options:
- Disabling unnecessary components (tools, tests, profiling, coverage)
- Explicitly disabling PNG support
- Disabling iconv dependency
These options help streamline the build and reduce unnecessary dependencies.
16-16: Updated preprocessing to apply the new patchThe preprocessing step now applies the new patch instead of using the previous method. This is consistent with the migration to CMake-based building.
21-21: CMake configuration commandThe configuration command has been replaced with a CMake invocation specifying both source and build directories as the current directory. This is standard practice for CMake projects.
32-34: Post-installation cleanup addedA post-processing step has been added to remove the
sharedirectory after installation. This is good practice to eliminate unnecessary files that won't be used.
1-34:✅ Verification successful
Overall transition to CMake build system is well-executed
The migration from autoconf to CMake has been implemented thoroughly with appropriate configuration options and build steps. The changes address the compilation issues mentioned in the PR objectives while maintaining a clean build process.
One thing to note: The AI summary mentions a new package group variable
qrencode_packages=qrencodewas added todepends/packages/packages.mk, but this change is not visible in the current file under review. Please ensure that change was properly implemented in the other file.
🏁 Script executed:
#!/bin/bash # Check if the packages.mk file contains the new package group variable if [ -f "depends/packages/packages.mk" ]; then echo "Checking for qrencode_packages in packages.mk" grep -n "qrencode_packages" depends/packages/packages.mk else echo "packages.mk file not found at expected location" fiLength of output: 229
qrencode_packages variable confirmed
The entry
qrencode_packages=qrencodeis present independs/packages/packages.mk:13. The CMake migration independs/packages/qrencode.mkis well-configured, and no further changes are needed. Approving the code changes.
6-6:✅ Verification successful
New patch file introduced
A new patch file
cmake_fixups.patchis now being referenced. According to the summary, this patch updates the CMakeLists.txt to fix the minimum required CMake version and corrects casing issues.
🏁 Script executed:
#!/bin/bash # Check if the patch file exists and examine its contents if [ -f "depends/patches/qrencode/cmake_fixups.patch" ]; then echo "Patch file exists" cat depends/patches/qrencode/cmake_fixups.patch else echo "Patch file does not exist at expected location" fiLength of output: 757
Patch Verification Complete – No Issues Found
The new patch file
depends/patches/qrencode/cmake_fixups.patchis present and correctly applied. It updatesCMakeLists.txtto:
- Bump
cmake_minimum_requiredfrom 3.1.0 to 3.5- Correct the casing of
find_package(ICONV)- Fix various developer warning messages
All changes match the intended fix. Approving these updates.
PR intention
QREncode library does not compile whether we want it or not. This fixes the issue.
#1557