Skip to content

fix incompatibility with bitcoin for qrencode #1697

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Oct 18, 2025
Merged

fix incompatibility with bitcoin for qrencode #1697

merged 1 commit into from
Oct 18, 2025

Conversation

@aleflm
Copy link
Contributor

@aleflm aleflm commented Oct 18, 2025

PR intention

fix incompatibility with bitcoin for qrencode

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 18, 2025 •
edited
Loading

Walkthrough

Replaced a host-OS-specific qrencode package variable with a generic qrencode_packages reference in depends/Makefile, and updated depends/packages/qrencode.mk to fetch libqrencode from the GitHub tags archive (v$(version).tar.gz) with a new checksum.

Changes

Cohort / File(s) Summary
Makefile qrencode dependency
depends/Makefile		 Swapped qrencode_$(host_os)_packages expansion for a generic qrencode_packages reference, changing how QR-related packages are selected/included.
qrencode package source and checksum
depends/packages/qrencode.mk		 Updated upstream source URL to GitHub tags archive (v$(version).tar.gz), adjusted expected archive filename, and replaced the SHA256 checksum to match the new source; build/patch stanzas unchanged.

Sequence Diagram(s)

sequenceDiagram
  participant Make as Makefile
  participant PKG as qrencode.mk
  participant Fetch as Remote (GitHub)
  Note over Make,PKG: Build dependency resolution
  Make->>PKG: reference `qrencode_packages`
  PKG->>Fetch: download "v$(version).tar.gz"
  Fetch-->>PKG: archive + checksum
  PKG->>Make: provide package build target
  Note right of Fetch #d6eaf8: Source host changed\nfrom fukuchi.org to GitHub
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Suggested reviewers

  • psolstice
  • levonpetrosyan93

Poem

🐰 A tiny hop from old to new,
Variables swapped, and sources too,
From fukuchi's field to GitHub's tag,
Checksums aligned, no trailing snag —
I nibble code, then dance and chew! 🥕

Pre-merge checks and finishing touches

❌ Failed checks (1 warning, 1 inconclusive)
Check name Status Explanation Resolution
Description Check ⚠️ Warning The PR description is severely incomplete and fails to meet the mandatory requirements of the template. The "PR intention" section, which is marked as mandatory and should explain what the PR does, what change it introduces, and what issue it solves, contains only a single line that merely restates the title without providing substantive detail. The description does not explain what the incompatibility is, why it matters, or how the code changes address it. Additionally, the optional "Code changes brief" section is entirely absent, leaving no explanation of the architectural or technical details of the modifications to qrencode and package selection logic. Expand the mandatory "PR intention" section to thoroughly describe the incompatibility issue, why it affects Bitcoin compatibility, and how the specific code changes (switching the qrencode source from fukuchi.org to GitHub and modifying package selection logic) resolve the problem. Consider adding the optional "Code changes brief" section to explain the architectural rationale for these changes. The description should provide enough context that reviewers understand both the problem and the solution without having to infer details from the code diff alone.
Title Check ❓ Inconclusive The PR title "fix incompatibility with bitcoin for qrencode" identifies the general intent and affected component but lacks specificity about the actual changes made. The title states what problem is being addressed (Bitcoin incompatibility) but does not describe the solution—specifically, that the qrencode upstream source is being switched from fukuchi.org to GitHub, or that the package selection logic is being simplified. A developer scanning the commit history would understand this is about fixing a qrencode issue but would need to read the full description to understand what technical changes were implemented. The title is vague rather than clearly summarizing the main changes in the changeset. Consider revising the title to be more specific about the actual changes, such as "Update qrencode to use libqrencode GitHub source" or "Simplify qrencode package selection for Bitcoin compatibility." This would help readers quickly understand what was changed in the code, not just what issue is being addressed. The title should ideally reflect the primary technical change rather than just the problem being solved.
✅ Passed checks (1 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5b69e11 and d5933e6.

📒 Files selected for processing (2)
  • depends/Makefile (1 hunks)
  • depends/packages/qrencode.mk (1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
  • GitHub Check: build-guix-x86_64-linux-gnu
  • GitHub Check: build-guix-x86_64-apple-darwin
  • GitHub Check: build-guix-aarch64-linux-gnu
  • GitHub Check: build-guix-arm64-apple-darwin
  • GitHub Check: build-mac-cmake
  • GitHub Check: build-guix-x86_64-w64-mingw32
  • GitHub Check: build-windows-cmake
  • GitHub Check: build-linux-cmake
  • GitHub Check: Create Source Distribution
🔇 Additional comments (2)
depends/Makefile (1)

159-159: Change verified: qrencode_packages is properly defined and correctly applied.

The qrencode_packages variable is defined in depends/packages/packages.mk:13 as qrencode=qrencode. The change from OS-specific selection to a generic package definition is intentional and consistent with the conditional pattern used for other dependencies (boost, libevent, bdb, sqlite). No OS-specific variants remain, confirming the platform-independent approach is complete.

depends/packages/qrencode.mk (1)

3-5: I can see the file content is confirmed and the initial URL accessibility check passed. However, I'm still awaiting the Python checksum verification result. Could you provide the output from the Python script that computed the SHA256 hash? It should show whether the expected hash matches the actual downloaded file.

Once I have that confirmation, I'll generate the final rewritten review comment.

Comment @coderabbitai help to get the list of available commands and usage tips.

@aleflm aleflm force-pushed the dev/aleflm/fix-incomp-bitcion branch from 5b69e11 to d5933e6 Compare October 18, 2025 16:28
@reubenyap reubenyap merged commit cd5267f into firoorg:master Oct 18, 2025
14 of 15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@levonpetrosyan93 levonpetrosyan93 levonpetrosyan93 approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@aleflm @levonpetrosyan93 @reubenyap