Include zap-cli for .deb and .rpm packages#1591
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR aims to integrate zap-cli within the built packages by removing the separate CLI packaging script and verifying its inclusion across all supported platforms. Key changes include:
- Removal of the pack-cli.js file and corresponding npm run pack:cli commands.
- Adjustments in build-release-package.js to reorder npm commands for building electron apps and zap-cli.
- Updates to GitHub workflows to verify zap-cli inclusion in Linux .deb/.rpm packages.
Reviewed Changes
Copilot reviewed 4 out of 6 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src-script/pack-cli.js | Removed the standalone script for packaging zap-cli. |
| src-script/build-release-package.js | Reordered npm commands and removed pack:cli calls for all platforms. |
| .github/workflows/release.yml | Added Linux package verification steps; Windows check condition remains. |
| .github/workflows/matter.yml | Replaced direct npm commands with a unified build script call and updated file renaming for Linux. |
Files not reviewed (2)
- package.json: Language not supported
- src-script/install-packages-ubuntu: Language not supported
Comments suppressed due to low confidence (3)
src-script/pack-cli.js:1
- The entire file is removed; please ensure that no processes or scripts reference pack-cli.js elsewhere in the codebase.
-'use strict'
.github/workflows/release.yml:284
- The condition for verifying the Windows x64 .zip package uses 'macos' instead of a Windows identifier. Consider updating it (e.g., to startsWith(matrix.os, 'win')).
if: startsWith(matrix.os, 'macos')
.github/workflows/matter.yml:94
- There is an inconsistency in naming: the build script outputs deb and rpm files with names different from those being renamed in the workflow (e.g., zap-linux-x64_64.rpm vs zap-linux-x86_64.rpm). Please align these names to avoid deployment issues.
mv dist/zap-linux-amd64.deb dist/zap-linux-x64.deb
dhchandw
force-pushed
the
task/betterPackaging
branch
from
e7b5a11 to
0688f1c
Compare
| console.log(`Building for Mac... Output: ${outputPath}`) | ||
| await scriptUtil.executeCmd({}, 'npm', ['run', 'pack:mac']) // Building electron app | ||
| await scriptUtil.executeCmd({}, 'npm', ['run', 'pkg:mac']) // Building zap-cli | ||
| await scriptUtil.executeCmd({}, 'npm', ['run', 'pack:cli:mac']) // Adding zap-cli to zip file |
There was a problem hiding this comment.
Why is cli being removed here?
There was a problem hiding this comment.
That script adds the zap cli to the zip package built by the electron builder. This is not required anymore since electron builder will bundle this in while packaging zap.
| await scriptUtil.executeCmd({}, 'npm', ['run', 'pack:win']) // Building electron app | ||
| await scriptUtil.executeCmd({}, 'npm', ['run', 'pkg:win']) // Building zap-cli | ||
| await scriptUtil.executeCmd({}, 'npm', ['run', 'pack:cli:win']) // Adding zap-cli to zip file | ||
| await scriptUtil.executeCmd({}, 'npm', ['run', 'pack:win']) // Building electron app |
There was a problem hiding this comment.
Avoid extra diff by placing it where it was.
There was a problem hiding this comment.
This is intentional, the cli should be built before the electron builder is called.
tecimovic
left a comment
tecimovic
left a comment
There was a problem hiding this comment.
Generally ok. I think we suffer from lack of common zapdev script.
We should have a single interface for all ZAP related things. Lately we started in most projects creating a uniform: src-script/zapdev.js, which is linked from a ./z from toplevel, and then that becomes the SINGLE interface to all this build logic.
Zap predates this methodology, so maybe we should retrofit?
This makes sense, will make the CI much cleaner as well. Will create an issue/ticket for this. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1591 +/- ##
==========================================
+ Coverage 66.93% 67.01% +0.08%
==========================================
Files 197 198 +1
Lines 21827 22249 +422
Branches 4817 4919 +102
==========================================
+ Hits 14609 14911 +302
- Misses 7218 7338 +120 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
dhchandw
force-pushed
the
task/betterPackaging
branch
4 times, most recently
from
32f6cab to
40104ed
Compare
- Clean up code and fix release.yml workflow - Add zap-cli to .deb and .rpm packages via afterPack hook in Electron Builder - Include zap-cli in .zip files via afterAllArtifactsBuild hook in Electron Builder - Package ARM64 zap-cli binary for macOS ARM64 zap release
dhchandw
force-pushed
the
task/betterPackaging
branch
from
40104ed to
59341f4
Compare
Closes #905