RNS-Things/reticulum-meshchatX
1
0
Fork 0
Code Issues 1 Pull Requests Actions Packages Projects Releases 8 Wiki Activity

Legacy support #23

Merged
Sudo-Ivan merged 7 commits from legacy-support into master 2025-12-06 05:34:20 +00:00
Conversation 3 Commits 7 Files Changed 4 +110 -11
Sudo-Ivan commented 2025-12-06 05:00:07 +00:00 (Migrated from github.com)
Copy Link

Potentially fixes #18 by using older dependency versions and Electron that supports older CPUs.

Legacy Builds

Node: 18
Python: 3.11
Electron: 30.0.8

Potentially fixes #18 by using older dependency versions and Electron that supports older CPUs. **Legacy Builds** Node: 18 Python: 3.11 Electron: 30.0.8
deepsource-io[bot] commented 2025-12-06 05:00:26 +00:00 (Migrated from github.com)
Copy Link

Here's the code health analysis summary for commits 9e1a8ce..d97676a. View details on DeepSource ↗.

Analysis Summary

AnalyzerStatusSummaryLink
DeepSource Python LogoPython✅ SuccessView Check ↗
DeepSource Docker LogoDocker✅ SuccessView Check ↗
💡 If you’re a repository administrator, you can configure the quality gates from the settings.
<div><p>Here's the code health analysis summary for commits <code>9e1a8ce..d97676a</code>. <a href="https://app.deepsource.com/gh/Sudo-Ivan/reticulum-meshchatX/run/04640625-8aea-4a7f-8286-a388039ba6d2/">View details on DeepSource</a>&nbsp;↗.</p><h3>Analysis Summary</h3><table><thead><tr><th>Analyzer</th><th>Status</th><th>Summary</th><th>Link</th></tr></thead><tbody><tr><td><img src="https://static.deepsource.com/analyzer_logos/python.svg" alt="DeepSource Python Logo" width="16px" height="16px"/><strong>Python</strong></td><td><span>✅&nbsp;</span><span>Success</span></td><td></td><td><a href="https://app.deepsource.com/gh/Sudo-Ivan/reticulum-meshchatX/run/04640625-8aea-4a7f-8286-a388039ba6d2/python/">View Check</a>&nbsp;↗</td></tr><tr><td><img src="https://static.deepsource.com/analyzer_logos/docker.svg" alt="DeepSource Docker Logo" width="16px" height="16px"/><strong>Docker</strong></td><td><span>✅&nbsp;</span><span>Success</span></td><td></td><td><a href="https://app.deepsource.com/gh/Sudo-Ivan/reticulum-meshchatX/run/04640625-8aea-4a7f-8286-a388039ba6d2/docker/">View Check</a>&nbsp;↗</td></tr></tbody></table><hr/><blockquote><div>💡 If you’re a repository administrator, you can configure the quality gates from the <a href="https://app.deepsource.com/gh/Sudo-Ivan/reticulum-meshchatX/settings/reporting">settings</a>.</div></blockquote></div>
github-actions[bot] commented 2025-12-06 05:15:56 +00:00 (Migrated from github.com)
Copy Link

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

OpenSSF Scorecard

PackageVersionScoreDetails
actions/actions/checkout 34e114876b0b11c390a56381ad16ebd13914f8d5 🟢 6.3
Details
CheckScoreReason
Maintained🟢 34 commit(s) and 0 issue activity found in the last 90 days -- score normalized to 3
CII-Best-Practices⚠️ 0no effort to earn an OpenSSF best practices badge detected
Code-Review🟢 10all changesets reviewed
Binary-Artifacts🟢 10no binaries found in the repo
Dangerous-Workflow🟢 10no dangerous workflow patterns detected
License🟢 10license file detected
Packaging⚠️ -1packaging workflow not detected
Token-Permissions⚠️ 0detected GitHub workflow tokens with excessive permissions
Signed-Releases⚠️ -1no releases found
Fuzzing⚠️ 0project is not fuzzed
Security-Policy🟢 9security policy file detected
Pinned-Dependencies🟢 3dependency not pinned by hash detected -- score normalized to 3
Branch-Protection🟢 5branch protection is not maximal on development and all release branches
Vulnerabilities🟢 91 existing vulnerabilities detected
SAST🟢 8SAST tool detected but not run on all commits
actions/actions/dependency-review-action 3c4e3dcb1aa7874d2c16be7d79418e9b7efd6261 🟢 7.6
Details
CheckScoreReason
Maintained🟢 1030 commit(s) and 0 issue activity found in the last 90 days -- score normalized to 10
Security-Policy🟢 9security policy file detected
Dangerous-Workflow🟢 10no dangerous workflow patterns detected
Code-Review🟢 10all changesets reviewed
Binary-Artifacts🟢 10no binaries found in the repo
Token-Permissions🟢 9detected GitHub workflow tokens with excessive permissions
Packaging⚠️ -1packaging workflow not detected
CII-Best-Practices⚠️ 0no effort to earn an OpenSSF best practices badge detected
Fuzzing⚠️ 0project is not fuzzed
License🟢 10license file detected
Pinned-Dependencies⚠️ 1dependency not pinned by hash detected -- score normalized to 1
Signed-Releases⚠️ -1no releases found
Branch-Protection🟢 6branch protection is not maximal on development and all release branches
Vulnerabilities🟢 64 existing vulnerabilities detected
SAST🟢 10SAST tool is run on all commits

Scanned Files

  • .github/workflows/dependency-review.yml
<h1>Dependency Review</h1> ✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.<h2>OpenSSF Scorecard</h2> <table><tr><th>Package</th><th>Version</th><th>Score</th><th>Details</th></tr> <tr><td><a href="https://github.com/actions/checkout"> actions/actions/checkout </a></td><td>34e114876b0b11c390a56381ad16ebd13914f8d5</td> <td>:green_circle: 6.3</td><td><details><summary>Details</summary><table><tr><th>Check</th><th>Score</th><th>Reason</th></tr><tr><td>Maintained</td><td>:green_circle: 3</td><td>4 commit(s) and 0 issue activity found in the last 90 days -- score normalized to 3</td></tr><tr><td>CII-Best-Practices</td><td>:warning: 0</td><td>no effort to earn an OpenSSF best practices badge detected</td></tr><tr><td>Code-Review</td><td>:green_circle: 10</td><td>all changesets reviewed</td></tr><tr><td>Binary-Artifacts</td><td>:green_circle: 10</td><td>no binaries found in the repo</td></tr><tr><td>Dangerous-Workflow</td><td>:green_circle: 10</td><td>no dangerous workflow patterns detected</td></tr><tr><td>License</td><td>:green_circle: 10</td><td>license file detected</td></tr><tr><td>Packaging</td><td>:warning: -1</td><td>packaging workflow not detected</td></tr><tr><td>Token-Permissions</td><td>:warning: 0</td><td>detected GitHub workflow tokens with excessive permissions</td></tr><tr><td>Signed-Releases</td><td>:warning: -1</td><td>no releases found</td></tr><tr><td>Fuzzing</td><td>:warning: 0</td><td>project is not fuzzed</td></tr><tr><td>Security-Policy</td><td>:green_circle: 9</td><td>security policy file detected</td></tr><tr><td>Pinned-Dependencies</td><td>:green_circle: 3</td><td>dependency not pinned by hash detected -- score normalized to 3</td></tr><tr><td>Branch-Protection</td><td>:green_circle: 5</td><td>branch protection is not maximal on development and all release branches</td></tr><tr><td>Vulnerabilities</td><td>:green_circle: 9</td><td>1 existing vulnerabilities detected</td></tr><tr><td>SAST</td><td>:green_circle: 8</td><td>SAST tool detected but not run on all commits</td></tr></table></details></td></tr> <tr><td><a href="https://github.com/actions/dependency-review-action"> actions/actions/dependency-review-action </a></td><td>3c4e3dcb1aa7874d2c16be7d79418e9b7efd6261</td> <td>:green_circle: 7.6</td><td><details><summary>Details</summary><table><tr><th>Check</th><th>Score</th><th>Reason</th></tr><tr><td>Maintained</td><td>:green_circle: 10</td><td>30 commit(s) and 0 issue activity found in the last 90 days -- score normalized to 10</td></tr><tr><td>Security-Policy</td><td>:green_circle: 9</td><td>security policy file detected</td></tr><tr><td>Dangerous-Workflow</td><td>:green_circle: 10</td><td>no dangerous workflow patterns detected</td></tr><tr><td>Code-Review</td><td>:green_circle: 10</td><td>all changesets reviewed</td></tr><tr><td>Binary-Artifacts</td><td>:green_circle: 10</td><td>no binaries found in the repo</td></tr><tr><td>Token-Permissions</td><td>:green_circle: 9</td><td>detected GitHub workflow tokens with excessive permissions</td></tr><tr><td>Packaging</td><td>:warning: -1</td><td>packaging workflow not detected</td></tr><tr><td>CII-Best-Practices</td><td>:warning: 0</td><td>no effort to earn an OpenSSF best practices badge detected</td></tr><tr><td>Fuzzing</td><td>:warning: 0</td><td>project is not fuzzed</td></tr><tr><td>License</td><td>:green_circle: 10</td><td>license file detected</td></tr><tr><td>Pinned-Dependencies</td><td>:warning: 1</td><td>dependency not pinned by hash detected -- score normalized to 1</td></tr><tr><td>Signed-Releases</td><td>:warning: -1</td><td>no releases found</td></tr><tr><td>Branch-Protection</td><td>:green_circle: 6</td><td>branch protection is not maximal on development and all release branches</td></tr><tr><td>Vulnerabilities</td><td>:green_circle: 6</td><td>4 existing vulnerabilities detected</td></tr><tr><td>SAST</td><td>:green_circle: 10</td><td>SAST tool is run on all commits</td></tr></table></details></td></tr> </table><h2>Scanned Files</h2> <ul><li>.github/workflows/dependency-review.yml</li></ul> <!-- dependency-review-pr-comment-marker -->
coderabbitai[bot] (Migrated from github.com) reviewed 2025-12-06 05:17:52 +00:00
coderabbitai[bot] (Migrated from github.com) left a comment
Copy Link

Actionable comments posted: 2

🧹 Nitpick comments (2)
Makefile (1)

1-1: Add missing all and test phony targets to satisfy conventions.

Static analysis (checkmake) flags missing all and test targets, which are conventionally expected in Makefiles even if not implemented. Consider adding placeholders or documenting their absence.

Consider adding these lines to the phony target declaration:

-.PHONY: install run develop clean build build-appimage build-exe dist sync-version wheel node_modules python build-docker run-docker electron-legacy build-appimage-legacy build-exe-legacy
+.PHONY: all test install run develop clean build build-appimage build-exe dist sync-version wheel node_modules python build-docker run-docker electron-legacy build-appimage-legacy build-exe-legacy
+
+all: build
+
+test:
+	@echo "No tests configured"
.github/workflows/build.yml (1)

130-138: Node.js-based JSON modification for Electron version is fragile; consider using jq for robustness.

Line 137 modifies package.json by invoking Node.js directly with inline code:

node -e "const fs=require('fs');const pkg=require('./package.json');pkg.devDependencies.electron='${{ matrix.electron_version }}';fs.writeFileSync('package.json', JSON.stringify(pkg,null,2));"

This approach is susceptible to:

  • Changes in package.json structure (e.g., if devDependencies is missing or renamed).
  • Serialization issues if package.json contains special characters.

Consider using jq (commonly available in GitHub Actions runners) for safer JSON manipulation:

jq --arg version "${{ matrix.electron_version }}" '.devDependencies.electron = $version' package.json > package.json.tmp && mv package.json.tmp package.json

Alternatively, if jq is not available, add error handling:

node -e "
  const fs = require('fs');
  const pkg = require('./package.json');
  if (!pkg.devDependencies) pkg.devDependencies = {};
  pkg.devDependencies.electron = '${{ matrix.electron_version }}';
  fs.writeFileSync('package.json', JSON.stringify(pkg, null, 2) + '\n');
" || { echo 'Failed to modify package.json'; exit 1; }
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 9e1a8ce180 and 9a9022ffb0.

📒 Files selected for processing (2)
  • .github/workflows/build.yml (5 hunks)
  • Makefile (2 hunks)
🧰 Additional context used
🪛 checkmake (0.2.2)
Makefile

[warning] 1-1: Missing required phony target "all"

(minphony)

[warning] 1-1: Missing required phony target "test"

(minphony)

⏰ 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). (5)
  • GitHub Check: Build Desktop (linux-legacy)
  • GitHub Check: Build Desktop (windows-legacy)
  • GitHub Check: Build Desktop (linux)
  • GitHub Check: Build Desktop (windows)
  • GitHub Check: Build Desktop (mac)
🔇 Additional comments (5)
.github/workflows/build.yml (4)

76-120: Missing macOS legacy variant; clarify whether this is intentional.

The PR adds windows-legacy and linux-legacy variants but does not include a mac-legacy build, despite the standard workflow building for mac (macOS-14). The PR description mentions the goal is to support older CPUs (e.g., on Debian 12), but it's unclear whether:

  • macOS systems are not affected by the CPU compatibility issue, or
  • macOS legacy support is planned for a future PR, or
  • This is an oversight.

Please clarify the intent: Does macOS require legacy Electron/Node support, or is this a deliberate omission?

If macOS legacy support is needed, add a matrix entry like:

          - name: mac-legacy
            os: macos-14
            node: 18
            python: "3.11"
            release_artifacts: "dist/*-mac-*.dmg"
            build_input: build_mac
            dist_script: dist:mac-universal
            variant: legacy
            electron_version: "30.0.8"

197-197: Conditional checks updated correctly to support linux-legacy.

The changes from matrix.name == 'linux' to startsWith(matrix.name, 'linux') correctly match both linux and linux-legacy builds. This is necessary for installing patchelf and building the Python wheel in legacy builds.

Also applies to: 204-204

50-50: Standardize Python versions across frontend and desktop build jobs.

The build_frontend job (line 50) uses Python 3.12, while build_desktop jobs use Python 3.13 (standard) or 3.11 (legacy). Both jobs run sync_version.py (lines 53 and 166), which uses only standard library modules and is compatible with all three versions. While there's no technical incompatibility issue here since the project supports Python 3.11+, the version inconsistency is unnecessary and could cause confusion.

Consider standardizing to Python 3.13 for all standard builds and 3.11 for legacy builds to keep the frontend and desktop build environments consistent.

Also applies to: 79-79, 88-88, 97-97

244-248: Glob patterns are correctly designed to support both standard and legacy variants; no false match risk.

The patterns are intentionally broad to match both standard and legacy artifacts. The dist-prebuilt build script via electron-builder generates:

  • Windows: ReticulumMeshChat-v*-win-installer.exe and ReticulumMeshChat-v*-win-portable.exe
  • Mac: ReticulumMeshChat-v*-mac-${arch}.dmg (e.g., mac-universal)
  • Linux: ReticulumMeshChat-v*-linux.AppImage and ReticulumMeshChat-v*-linux.deb

The "Rename artifacts for legacy build" step explicitly appends -legacy before the extension for legacy variants, creating files like ReticulumMeshChat-v*-win-installer-legacy.exe. The wildcard patterns (*-win-installer*.exe, *-linux*.AppImage, etc.) capture both variants as intended. No false positives occur because electron-builder's artifact naming is deterministic and controlled by the configuration.

Makefile (1)

6-6: Keep Electron version in sync with workflow matrix definition.

The LEGACY_ELECTRON_VERSION default (30.0.8) currently matches the version in the workflow's legacy build matrix. Ensure this value remains synchronized if either location is updated.

**Actionable comments posted: 2** <details> <summary>🧹 Nitpick comments (2)</summary><blockquote> <details> <summary>Makefile (1)</summary><blockquote> `1-1`: **Add missing `all` and `test` phony targets to satisfy conventions.** Static analysis (checkmake) flags missing `all` and `test` targets, which are conventionally expected in Makefiles even if not implemented. Consider adding placeholders or documenting their absence. Consider adding these lines to the phony target declaration: ```diff -.PHONY: install run develop clean build build-appimage build-exe dist sync-version wheel node_modules python build-docker run-docker electron-legacy build-appimage-legacy build-exe-legacy +.PHONY: all test install run develop clean build build-appimage build-exe dist sync-version wheel node_modules python build-docker run-docker electron-legacy build-appimage-legacy build-exe-legacy + +all: build + +test: + @echo "No tests configured" ``` </blockquote></details> <details> <summary>.github/workflows/build.yml (1)</summary><blockquote> `130-138`: **Node.js-based JSON modification for Electron version is fragile; consider using `jq` for robustness.** Line 137 modifies `package.json` by invoking Node.js directly with inline code: ```javascript node -e "const fs=require('fs');const pkg=require('./package.json');pkg.devDependencies.electron='${{ matrix.electron_version }}';fs.writeFileSync('package.json', JSON.stringify(pkg,null,2));" ``` This approach is susceptible to: - Changes in `package.json` structure (e.g., if `devDependencies` is missing or renamed). - Serialization issues if `package.json` contains special characters. Consider using `jq` (commonly available in GitHub Actions runners) for safer JSON manipulation: ```bash jq --arg version "${{ matrix.electron_version }}" '.devDependencies.electron = $version' package.json > package.json.tmp && mv package.json.tmp package.json ``` Alternatively, if jq is not available, add error handling: ```bash node -e " const fs = require('fs'); const pkg = require('./package.json'); if (!pkg.devDependencies) pkg.devDependencies = {}; pkg.devDependencies.electron = '${{ matrix.electron_version }}'; fs.writeFileSync('package.json', JSON.stringify(pkg, null, 2) + '\n'); " || { echo 'Failed to modify package.json'; exit 1; } ``` </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used**: CodeRabbit UI **Review profile**: CHILL **Plan**: Pro <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 9e1a8ce1808fc6f165635cdc45e7cb9d616c0602 and 9a9022ffb06fdc8dd5edac40ba62f03e8e3db92b. </details> <details> <summary>📒 Files selected for processing (2)</summary> * `.github/workflows/build.yml` (5 hunks) * `Makefile` (2 hunks) </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🪛 checkmake (0.2.2)</summary> <details> <summary>Makefile</summary> [warning] 1-1: Missing required phony target "all" (minphony) --- [warning] 1-1: Missing required phony target "test" (minphony) </details> </details> </details> <details> <summary>⏰ 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). (5)</summary> * GitHub Check: Build Desktop (linux-legacy) * GitHub Check: Build Desktop (windows-legacy) * GitHub Check: Build Desktop (linux) * GitHub Check: Build Desktop (windows) * GitHub Check: Build Desktop (mac) </details> <details> <summary>🔇 Additional comments (5)</summary><blockquote> <details> <summary>.github/workflows/build.yml (4)</summary><blockquote> `76-120`: **Missing macOS legacy variant; clarify whether this is intentional.** The PR adds `windows-legacy` and `linux-legacy` variants but does not include a `mac-legacy` build, despite the standard workflow building for `mac` (macOS-14). The PR description mentions the goal is to support older CPUs (e.g., on Debian 12), but it's unclear whether: - macOS systems are not affected by the CPU compatibility issue, or - macOS legacy support is planned for a future PR, or - This is an oversight. Please clarify the intent: Does macOS require legacy Electron/Node support, or is this a deliberate omission? If macOS legacy support is needed, add a matrix entry like: ```yaml - name: mac-legacy os: macos-14 node: 18 python: "3.11" release_artifacts: "dist/*-mac-*.dmg" build_input: build_mac dist_script: dist:mac-universal variant: legacy electron_version: "30.0.8" ``` --- `197-197`: **Conditional checks updated correctly to support linux-legacy.** The changes from `matrix.name == 'linux'` to `startsWith(matrix.name, 'linux')` correctly match both `linux` and `linux-legacy` builds. This is necessary for installing patchelf and building the Python wheel in legacy builds. Also applies to: 204-204 --- `50-50`: **Standardize Python versions across frontend and desktop build jobs.** The `build_frontend` job (line 50) uses Python 3.12, while `build_desktop` jobs use Python 3.13 (standard) or 3.11 (legacy). Both jobs run `sync_version.py` (lines 53 and 166), which uses only standard library modules and is compatible with all three versions. While there's no technical incompatibility issue here since the project supports Python 3.11+, the version inconsistency is unnecessary and could cause confusion. Consider standardizing to Python 3.13 for all standard builds and 3.11 for legacy builds to keep the frontend and desktop build environments consistent. Also applies to: 79-79, 88-88, 97-97 --- `244-248`: **Glob patterns are correctly designed to support both standard and legacy variants; no false match risk.** The patterns are intentionally broad to match both standard and legacy artifacts. The `dist-prebuilt` build script via electron-builder generates: - Windows: `ReticulumMeshChat-v*-win-installer.exe` and `ReticulumMeshChat-v*-win-portable.exe` - Mac: `ReticulumMeshChat-v*-mac-${arch}.dmg` (e.g., `mac-universal`) - Linux: `ReticulumMeshChat-v*-linux.AppImage` and `ReticulumMeshChat-v*-linux.deb` The "Rename artifacts for legacy build" step explicitly appends `-legacy` before the extension for legacy variants, creating files like `ReticulumMeshChat-v*-win-installer-legacy.exe`. The wildcard patterns (`*-win-installer*.exe`, `*-linux*.AppImage`, etc.) capture both variants as intended. No false positives occur because electron-builder's artifact naming is deterministic and controlled by the configuration. </blockquote></details> <details> <summary>Makefile (1)</summary><blockquote> `6-6`: **Keep Electron version in sync with workflow matrix definition.** The `LEGACY_ELECTRON_VERSION` default (30.0.8) currently matches the version in the workflow's legacy build matrix. Ensure this value remains synchronized if either location is updated. </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
.github/workflows/build.yml
@@ -182,6 +216,13 @@ jobs:
(github.event_name == 'workflow_dispatch' && inputs[matrix.build_input] == true)
coderabbitai[bot] (Migrated from github.com) commented 2025-12-06 05:17:51 +00:00
Copy Link

🛠️ Refactor suggestion | 🟠 Major

Artifact renaming logic duplicates Makefile implementation; extract to shared script for maintainability.

The "Rename artifacts for legacy build" step duplicates the renaming logic from the Makefile's build-appimage-legacy and build-exe-legacy targets. The workflow version:

  • Uses bash shopt -s nullglob for safe glob matching (good practice).
  • Renames all legacy artifacts with a -legacy suffix.

The Makefile version does the same but is called only if the Makefile targets are invoked directly (which the workflow does not do).

Recommendation: Extract the renaming logic into a shared shell script (e.g., scripts/rename_legacy_artifacts.sh) and invoke it from both the Makefile and the workflow:

# scripts/rename_legacy_artifacts.sh
#!/bin/bash
set -e
shopt -s nullglob
for f in dist/*-win-installer.exe dist/*-win-portable.exe dist/*-linux.AppImage dist/*-linux.deb; do
  [ -e "$f" ] || continue
  dir=$(dirname "$f")
  base=$(basename "$f")
  ext="${base##*.}"
  name="${base%.$ext}"
  mv "$f" "$dir/${name}-legacy.${ext}"
done

Then in the workflow (line 225–234):

run: bash scripts/rename_legacy_artifacts.sh

And in the Makefile (lines 54–58, 63–67):

	bash scripts/rename_legacy_artifacts.sh

This eliminates duplication and makes future updates easier.

🤖 Prompt for AI Agents 
.github/workflows/build.yml lines 219-234: the workflow duplicates Makefile
artifact-renaming logic—extract that loop into a shared executable script (e.g.,
scripts/rename_legacy_artifacts.sh) containing the bash renaming logic, make it
executable, replace the inline run block in this workflow with a call to execute
that script, and update the corresponding Makefile targets to call the same
script instead of duplicating the loop so both the CI workflow and Makefile
reuse the single canonical implementation.

✅ Addressed in commits bf8c22c to d97676a

_🛠️ Refactor suggestion_ | _🟠 Major_ **Artifact renaming logic duplicates Makefile implementation; extract to shared script for maintainability.** The "Rename artifacts for legacy build" step duplicates the renaming logic from the Makefile's `build-appimage-legacy` and `build-exe-legacy` targets. The workflow version: - Uses bash `shopt -s nullglob` for safe glob matching (good practice). - Renames all legacy artifacts with a `-legacy` suffix. The Makefile version does the same but is called only if the Makefile targets are invoked directly (which the workflow does not do). **Recommendation:** Extract the renaming logic into a shared shell script (e.g., `scripts/rename_legacy_artifacts.sh`) and invoke it from both the Makefile and the workflow: ```bash # scripts/rename_legacy_artifacts.sh #!/bin/bash set -e shopt -s nullglob for f in dist/*-win-installer.exe dist/*-win-portable.exe dist/*-linux.AppImage dist/*-linux.deb; do [ -e "$f" ] || continue dir=$(dirname "$f") base=$(basename "$f") ext="${base##*.}" name="${base%.$ext}" mv "$f" "$dir/${name}-legacy.${ext}" done ``` Then in the workflow (line 225–234): ```yaml run: bash scripts/rename_legacy_artifacts.sh ``` And in the Makefile (lines 54–58, 63–67): ```makefile bash scripts/rename_legacy_artifacts.sh ``` This eliminates duplication and makes future updates easier. <details> <summary>🤖 Prompt for AI Agents</summary> ``` .github/workflows/build.yml lines 219-234: the workflow duplicates Makefile artifact-renaming logic—extract that loop into a shared executable script (e.g., scripts/rename_legacy_artifacts.sh) containing the bash renaming logic, make it executable, replace the inline run block in this workflow with a call to execute that script, and update the corresponding Makefile targets to call the same script instead of duplicating the loop so both the CI workflow and Makefile reuse the single canonical implementation. ``` </details> <!-- fingerprinting:phantom:triton:mongoose --> <!-- This is an auto-generated comment by CodeRabbit --> ✅ Addressed in commits bf8c22c to d97676a
Makefile
@@ -44,6 +45,20 @@ build-exe: build
coderabbitai[bot] (Migrated from github.com) commented 2025-12-06 05:17:51 +00:00
Copy Link

🛠️ Refactor suggestion | 🟠 Major

Artifact renaming logic is duplicated between Makefile and workflow; clarify intended usage.

The artifact renaming logic (bash pattern matching to append -legacy suffix) appears identically in both:

  • Makefile: lines 54–58 (AppImage/deb) and 63–67 (Windows exe)
  • Workflow: lines 226–234 (all platforms)

The workflow does not invoke these Makefile targets; it runs npm commands directly and performs its own renaming. This duplication complicates maintenance—if the renaming logic needs to change, updates must be made in two places.

Recommendation: If the Makefile targets are meant only for local development, add a comment documenting this. Alternatively, consider extracting the renaming logic into a shared script that both the Makefile and workflow invoke, or remove the Makefile targets if they are unused in the actual CI/CD pipeline.

For now, consider adding a comment above the legacy targets:

+# Legacy build targets for local development.
+# Note: The GitHub Actions workflow does not invoke these targets;
+# it runs npm commands directly and performs artifact renaming inline.
 electron-legacy:
 	$(NPM) install --no-save electron@$(LEGACY_ELECTRON_VERSION)

Also, consider handling the case where no files match the glob pattern. The workflow uses shopt -s nullglob to avoid errors if no files match, but the Makefile targets do not. Add the nullglob option to the Makefile loops:

 build-appimage-legacy: build electron-legacy
 	$(NPM) run electron-postinstall
 	$(NPM) run dist -- --linux AppImage
-	@set -e; for f in dist/*-linux.AppImage dist/*-linux.deb; do \
+	@set -e; shopt -s nullglob; for f in dist/*-linux.AppImage dist/*-linux.deb; do \
 		[ -e "$$f" ] || continue; \
 		dir=$$(dirname "$$f"); base=$$(basename "$$f"); ext=$${base##*.}; name=$${base%.$$ext}; \
 		mv "$$f" "$$dir/$${name}-legacy.$$ext"; \
 	done

Note: The [ -e "$$f" ] check makes the nullglob redundant, so this is a minor improvement for clarity. The current code is safe.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

# Legacy build targets for local development.
# Note: The GitHub Actions workflow does not invoke these targets;
# it runs npm commands directly and performs artifact renaming inline.
electron-legacy:
	$(NPM) install --no-save electron@$(LEGACY_ELECTRON_VERSION)

build-appimage-legacy: build electron-legacy
	$(NPM) run electron-postinstall
	$(NPM) run dist -- --linux AppImage
	@set -e; shopt -s nullglob; for f in dist/*-linux.AppImage dist/*-linux.deb; do \
		[ -e "$$f" ] || continue; \
		dir=$$(dirname "$$f"); base=$$(basename "$$f"); ext=$${base##*.}; name=$${base%.$$ext}; \
		mv "$$f" "$$dir/$${name}-legacy.$$ext"; \
	done

build-exe-legacy: build electron-legacy
	$(NPM) run electron-postinstall
	$(NPM) run dist -- --win portable
	@set -e; shopt -s nullglob; for f in dist/*-win-installer.exe dist/*-win-portable.exe; do \
		[ -e "$$f" ] || continue; \
		dir=$$(dirname "$$f"); base=$$(basename "$$f"); ext=$${base##*.}; name=$${base%.$$ext}; \
		mv "$$f" "$$dir/$${name}-legacy.$$ext"; \
	done
🤖 Prompt for AI Agents 
In Makefile around lines 48–67 the artifact-renaming bash loops for legacy
builds duplicate logic that also exists in the CI workflow and lack an explicit
note about intended usage; update this section by adding a brief comment that
these legacy targets are intended for local/manual use (or remove them if unused
in CI), and refactor the renaming into a single shared script (e.g.,
scripts/rename-legacy-artifacts.sh) that both the Makefile targets and the
workflow invoke to avoid duplication; additionally, make the Makefile loop
robust by enabling nullglob in the shell invocation or otherwise ensuring no-op
behavior when globs match nothing (the existing [ -e "$$f" ] guard is acceptable
but document the choice).

✅ Addressed in commits bf8c22c to d97676a

_🛠️ Refactor suggestion_ | _🟠 Major_ **Artifact renaming logic is duplicated between Makefile and workflow; clarify intended usage.** The artifact renaming logic (bash pattern matching to append `-legacy` suffix) appears identically in both: - Makefile: lines 54–58 (AppImage/deb) and 63–67 (Windows exe) - Workflow: lines 226–234 (all platforms) The workflow does not invoke these Makefile targets; it runs npm commands directly and performs its own renaming. This duplication complicates maintenance—if the renaming logic needs to change, updates must be made in two places. **Recommendation:** If the Makefile targets are meant only for local development, add a comment documenting this. Alternatively, consider extracting the renaming logic into a shared script that both the Makefile and workflow invoke, or remove the Makefile targets if they are unused in the actual CI/CD pipeline. For now, consider adding a comment above the legacy targets: ```diff +# Legacy build targets for local development. +# Note: The GitHub Actions workflow does not invoke these targets; +# it runs npm commands directly and performs artifact renaming inline. electron-legacy: $(NPM) install --no-save electron@$(LEGACY_ELECTRON_VERSION) ``` **Also,** consider handling the case where no files match the glob pattern. The workflow uses `shopt -s nullglob` to avoid errors if no files match, but the Makefile targets do not. Add the nullglob option to the Makefile loops: ```diff build-appimage-legacy: build electron-legacy $(NPM) run electron-postinstall $(NPM) run dist -- --linux AppImage - @set -e; for f in dist/*-linux.AppImage dist/*-linux.deb; do \ + @set -e; shopt -s nullglob; for f in dist/*-linux.AppImage dist/*-linux.deb; do \ [ -e "$$f" ] || continue; \ dir=$$(dirname "$$f"); base=$$(basename "$$f"); ext=$${base##*.}; name=$${base%.$$ext}; \ mv "$$f" "$$dir/$${name}-legacy.$$ext"; \ done ``` Note: The `[ -e "$$f" ]` check makes the nullglob redundant, so this is a minor improvement for clarity. The current code is safe. <!-- suggestion_start --> <details> <summary>📝 Committable suggestion</summary> > ‼️ **IMPORTANT** > Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements. ```suggestion # Legacy build targets for local development. # Note: The GitHub Actions workflow does not invoke these targets; # it runs npm commands directly and performs artifact renaming inline. electron-legacy: $(NPM) install --no-save electron@$(LEGACY_ELECTRON_VERSION) build-appimage-legacy: build electron-legacy $(NPM) run electron-postinstall $(NPM) run dist -- --linux AppImage @set -e; shopt -s nullglob; for f in dist/*-linux.AppImage dist/*-linux.deb; do \ [ -e "$$f" ] || continue; \ dir=$$(dirname "$$f"); base=$$(basename "$$f"); ext=$${base##*.}; name=$${base%.$$ext}; \ mv "$$f" "$$dir/$${name}-legacy.$$ext"; \ done build-exe-legacy: build electron-legacy $(NPM) run electron-postinstall $(NPM) run dist -- --win portable @set -e; shopt -s nullglob; for f in dist/*-win-installer.exe dist/*-win-portable.exe; do \ [ -e "$$f" ] || continue; \ dir=$$(dirname "$$f"); base=$$(basename "$$f"); ext=$${base##*.}; name=$${base%.$$ext}; \ mv "$$f" "$$dir/$${name}-legacy.$$ext"; \ done ``` </details> <!-- suggestion_end --> <details> <summary>🤖 Prompt for AI Agents</summary> ``` In Makefile around lines 48–67 the artifact-renaming bash loops for legacy builds duplicate logic that also exists in the CI workflow and lack an explicit note about intended usage; update this section by adding a brief comment that these legacy targets are intended for local/manual use (or remove them if unused in CI), and refactor the renaming into a single shared script (e.g., scripts/rename-legacy-artifacts.sh) that both the Makefile targets and the workflow invoke to avoid duplication; additionally, make the Makefile loop robust by enabling nullglob in the shell invocation or otherwise ensuring no-op behavior when globs match nothing (the existing [ -e "$$f" ] guard is acceptable but document the choice). ``` </details> <!-- fingerprinting:phantom:triton:mongoose --> <!-- This is an auto-generated comment by CodeRabbit --> ✅ Addressed in commits bf8c22c to d97676a
Sign in to join this conversation.
Reviewers
No Reviewers
coderabbitai[bot]
Labels
No items
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
Ivan
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: RNS-Things/reticulum-meshchatX#23
Delete Branch "legacy-support"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?