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 +23 -1
1 changed files with 23 additions and 1 deletions
Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 0443734ee3 - Show all commits

24
Makefile
View File

@@ -1,8 +1,9 @@
.PHONY: install run develop clean build build-appimage build-exe dist sync-version wheel node_modules python build-docker run-docker
.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
PYTHON ?= python
POETRY = $(PYTHON) -m poetry
NPM = npm
LEGACY_ELECTRON_VERSION ?= 30.0.8
DOCKER_COMPOSE_CMD ?= docker compose
DOCKER_COMPOSE_FILE ?= docker-compose.yml
@@ -44,6 +45,27 @@ build-exe: build
coderabbitai[bot] commented 2025-12-06 05:17:51 +00:00 (Migrated from github.com)
Review
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
dist: build-appimage
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; 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; 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
clean:
rm -rf node_modules
rm -rf build