Skip to content

Conversation

@Eeems
Copy link
Collaborator

@Eeems Eeems commented Oct 25, 2025

Summary by CodeRabbit

  • Chores
    • Improved cross-platform build and test compatibility
    • Updated platform-specific dependencies to latest versions

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 25, 2025

Walkthrough

Add portable SHA-256 verifier in the Makefile by detecting platform and setting a SHA256SUM variable; use it in test targets. Bump two platform-specific package versions in requirements.txt.

Changes

Cohort / File(s) Summary
Makefile: hash verification
Makefile
Add UNAME_S detection and set SHA256SUM to gsha256sum on Darwin, otherwise sha256sum; replace direct sha256sum --check calls with $(SHA256SUM) -c in test and test-executable targets.
Dependency bumps
requirements.txt
Update platform-specific package versions: remarkable-update-image 1.1.6 → 1.1.7 (non-Linux), remarkable-update-fuse 1.2.4 → 1.2.5 (Linux).

Sequence Diagram(s)

sequenceDiagram
    autonumber
    participant Makefile
    participant Shell as /bin/sh
    participant SHA as SHA256SUM
    rect rgba(230,247,255,0.6)
    Makefile->>Shell: invoke test / test-executable
    Shell->>Makefile: evaluate UNAME_S
    Makefile->>SHA: set SHA256SUM (gsha256sum on Darwin, else sha256sum)
    Makefile->>Shell: run $(SHA256SUM) -c file.sha256
    alt verifier present and check succeeds
        SHA-->>Shell: 0 (success)
        Shell-->>Makefile: continue
    else verifier present and check fails
        SHA-->>Shell: non-zero (fail)
        Shell-->>Makefile: stop / fail target
    else verifier missing
        Shell-->>Makefile: error and exit 1
    end
    end
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

  • Review the two Makefile locations to confirm $(SHA256SUM) usage and correct file path/variable names.
  • Verify the platform conditional (Darwin detection) and that gsha256sum is the intended macOS binary.
  • Confirm the requirements.txt platform markers and version strings match packaging conventions.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The pull request title "Update remarkable-update-image and remarkable-update-fuse versions" accurately describes the requirements.txt changes, which involve updating two dependency versions. However, the changeset also includes significant Makefile modifications that introduce cross-platform SHA-256 verification logic. The title refers to a real and specific part of the change but does not capture the complete scope of the pull request, as it omits the infrastructure improvements in the build system.
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch Eeems-patch-2

📜 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 507a25c and 9c2f81d.

📒 Files selected for processing (1)
  • Makefile (3 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). (4)
  • GitHub Check: Build for macos-latest
  • GitHub Check: Build for ubuntu-latest
  • GitHub Check: Build for windows-latest
  • GitHub Check: Build for remarkable
🔇 Additional comments (2)
Makefile (2)

22-27: Cross-platform SHA-256 verification is correctly implemented.

The platform detection logic correctly sets SHA256SUM to gsha256sum on Darwin and sha256sum on other Unix-like systems. This is a clean and idiomatic approach to handling platform differences in Makefiles.


67-67: Proper variable substitution in both test targets.

The usage of $(SHA256SUM) in both the test and test-executable targets correctly applies the platform-aware variable. Both commands use the -c flag appropriately, which works identically across sha256sum and gsha256sum.

Also applies to: 83-83


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
Makefile (1)

60-67: Code duplication: extract hash verification logic into a variable.

The same 8-line hash verification block appears in both test (lines 60–67) and test-executable (lines 83–90) targets. Consider defining a Makefile variable to eliminate duplication and simplify maintenance.

Define a variable at the top of the Makefile:

CHECK_IMG_SHA := if command -v sha256sum >/dev/null 2>&1; then \
  echo "$${IMG_SHA}  .venv/$${FW_VERSION}_reMarkable2-$${FW_DATA}.img" | sha256sum --check; \
elif command -v shasum >/dev/null 2>&1; then \
  echo "$${IMG_SHA}  .venv/$${FW_VERSION}_reMarkable2-$${FW_DATA}.img" | shasum -a 256 --check; \
else \
  echo "Error: Neither sha256sum nor shasum is available."; \
  exit 1; \
fi

Then replace each occurrence with:

	$(CHECK_IMG_SHA); \

Note: Escape $ as $$ in variable definitions to preserve shell variable references.

Also applies to: 83-90

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 515dc57 and f7aa581.

📒 Files selected for processing (2)
  • Makefile (2 hunks)
  • requirements.txt (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). (3)
  • GitHub Check: Build for ubuntu-latest
  • GitHub Check: Build for remarkable
  • GitHub Check: Build for windows-latest
🔇 Additional comments (2)
requirements.txt (1)

3-4: Platform-specific dependency versions updated correctly.

The version bumps for remarkable-update-image and remarkable-update-fuse maintain the platform-specific conditions and follow the expected format.

Makefile (1)

60-67: Hash verification logic verified; cross-platform fallback is compatible.

The shasum -a 256 --check command accepts the same checksum file format as sha256sum --check, confirming that this portable fallback implementation works correctly across platforms.

@Eeems Eeems merged commit f5c6ba5 into main Oct 25, 2025
8 checks passed
@Eeems Eeems deleted the Eeems-patch-2 branch October 25, 2025 23:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants