Repository navigation
feat: add npm binary distribution support - #3
Conversation
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughAdds platform-specific npm packages and publishing (macOS, Linux, Windows x64/arm64), a Node CLI wrapper that resolves and execs the platform binary, CI/release workflow updates to build/publish per-platform packages, install script improvements, helper scripts, and documentation/manifest updates. Changes
Sequence Diagram(s)sequenceDiagram
actor Developer
participant GH as GitHub Actions
participant Builder as Build Matrix (per-platform)
participant Packager as Release packager
participant NPM as npm Registry
Developer->>GH: trigger release
GH->>Builder: run builds (matrix entries, npm_platform)
Builder->>GH: upload artifacts
GH->>Packager: for each platform, copy/rename binary (append `.exe` on Windows)
Packager->>NPM: publish platform package (`npm/platforms/<platform>`)
GH->>NPM: publish main package (`npm/skeletor`) gated by publish_npm
sequenceDiagram
actor User
participant CLI as `npm/skeletor/bin/skeletor` (Node wrapper)
participant NodeMods as `node_modules/@geekist/*`
participant LocalBin as Monorepo binary path
participant Native as Native binary (skeletor or skeletor.exe)
User->>CLI: invoke `skeletor` with args
CLI->>NodeMods: map platform -> optionalDependency and attempt resolve
alt dependency found
CLI->>Native: execute resolved binary path with args
else fallback
CLI->>LocalBin: attempt monorepo binary path
CLI->>Native: execute local binary
end
Native-->>CLI: exit code / stdio
CLI-->>User: propagate stdio and exit code
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📥 CommitsReviewing files that changed from the base of the PR and between 8a077b47a1ccabcd3f29de0dc840d75c36b0efb4 and f4ea2fd8b1a141afe1af0b696f4dc5e613e6acf8. 📒 Files selected for processing (15)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/release.yml (1)
81-95: Add NPM_TOKEN validation to preflight checks.The workflow uses
NPM_TOKENfor publishing platform-specific and main npm packages, but this secret is not validated in the preflight job likeCARGO_REGISTRY_TOKENandGH_PATare. This could lead to workflow failures during the publish step.🔧 Proposed fix
- name: Check required secrets env: CARGO_REGISTRY_TOKEN: ${{ secrets.CARGO_REGISTRY_TOKEN }} GH_PAT: ${{ secrets.GH_PAT }} + NPM_TOKEN: ${{ secrets.NPM_TOKEN }} shell: bash run: | set -Eeuo pipefail if [[ "${{ steps.vars.outputs.publish_crates }}" == "true" ]] && [[ -z "${CARGO_REGISTRY_TOKEN:-}" ]]; then echo "CARGO_REGISTRY_TOKEN is missing but publish_crates=true" exit 1 fi if [[ "${{ steps.vars.outputs.update_tap }}" == "true" ]] && [[ -z "${GH_PAT:-}" ]]; then echo "GH_PAT is missing but update_tap=true" exit 1 fi + if [[ "${{ steps.vars.outputs.publish_crates }}" == "true" ]] && [[ -z "${NPM_TOKEN:-}" ]]; then + echo "NPM_TOKEN is missing but npm publishing is enabled" + exit 1 + fi
🤖 Fix all issues with AI agents
In @install.sh:
- Around line 131-140: The install logic fails to account for /usr/local/bin not
existing, which can cause installation to target a non-existent directory when
running as root; update the branch that handles the root/else path in install.sh
(the block referencing INSTALL_DIR and USE_SUDO) to first ensure the intended
system install directory exists (e.g., verify/create /usr/local/bin with mkdir
-p when EUID==0) before checking writability, and/or adjust INSTALL_DIR to a
valid path; ensure the writability check uses the actual directory being
installed into and only sets USE_SUDO=true after confirming the directory
exists.
In @npm/skeletor/bin/skeletor:
- Line 51: Update the console.error call that prints the package installation
message: change the string literal passed to console.error from "Please Ensure
that ${packageName} is installed." to use a lowercase 'e' ("Please ensure that
${packageName} is installed.") so the message reads correctly; the change is in
the console.error(...) invocation referencing packageName.
- Around line 58-60: The child close handler currently calls process.exit(code)
which treats a null code (process killed by signal) as 0; change the listener
signature to (code, signal) and if code is not null call process.exit(code),
otherwise log the signal and exit with a non‑zero code (e.g., process.exit(1))
so a signal termination is reflected as failure instead of success; update the
child.on('close', (code) => ...) callback accordingly to use the new parameters
and logic.
In @npm/skeletor/package.json:
- Around line 16-18: The package.json declares a "postinstall" npm script that
references a missing scripts/postinstall.js, causing npm install to fail; either
remove the "postinstall" entry from the "scripts" object in package.json or add
a new scripts/postinstall.js file implementing the intended post-install
behavior (e.g., any setup or asset copying) and ensure it is executable and
referenced correctly by the "postinstall" script name.
- Around line 8-15: The optionalDependencies block currently lists packages
under the @skeletor scope (e.g., "@skeletor/darwin-arm64",
"@skeletor/win32-x64"), which don't match the published package names; update
each key in the "optionalDependencies" map to the correct published names under
the @geekist scope (e.g., change "@skeletor/darwin-arm64" →
"@geekist/skeletor-darwin-arm64", etc.), keeping the same versions, so npm can
resolve the packages during install.
🧹 Nitpick comments (12)
npm/platforms/win32-arm64/package.json (1)
1-14: AddpublishConfig.access(or ensure workflow uses--access public) + basic package metadata (license,repository).
Scoped packages often fail publishing or end up restricted without explicit public access, and missing license metadata can be a compliance blocker.Proposed patch
{ "name": "@skeletor/win32-arm64", "version": "0.3.10", "description": "win32-arm64 binary for skeletor", + "license": "MIT", + "repository": { + "type": "git", + "url": "git+https://github.com/theGeekist/skeletor.git" + }, + "publishConfig": { + "access": "public" + }, "os": [ "win32" ], "cpu": [ "arm64" ], "files": [ "bin" ] }npm/platforms/darwin-arm64/package.json (1)
1-14: AddpublishConfig.access(or ensure workflow uses--access public) + basic package metadata (license,repository).
Same publish/compliance considerations as the other platform packages.npm/platforms/linux-arm64/package.json (1)
1-14: AddpublishConfig.access(or ensure workflow uses--access public) + basic package metadata (license,repository).
Same publish/compliance considerations as the other platform packages.npm/platforms/win32-x64/package.json (1)
1-14: AddpublishConfig.access(or ensure workflow uses--access public) + basic package metadata (license,repository).
Same publish/compliance considerations as the other platform packages.npm/skeletor/bin/skeletor (1)
35-37: Consider adding explanatory comments to empty catch blocks.Empty catch blocks without comments can make it unclear whether exceptions are intentionally ignored or if error handling was overlooked.
♻️ Proposed improvement
} catch (e) { - // ignore + // Ignore resolution failures; will try fallback path } // Fallback for local development or if resolution fails // Try to find it relative to this script if we are in the monorepo structure // This might need adjustment depending on how exactly things are laid out when linked try { const ext = process.platform === 'win32' ? '.exe' : ''; const localBinaryPath = path.resolve(__dirname, '../../platforms', platformKey, 'bin', 'skeletor' + ext); if (fs.existsSync(localBinaryPath)) { return localBinaryPath; } - } catch (e) { } + } catch (e) { + // Ignore; will fall through to error below + }Also applies to: 48-48
npm/platforms/darwin-x64/package.json (1)
1-14: Add standard npm package metadata fields.The package manifest is missing common npm package fields such as
license,repository,author, andhomepage. These fields improve discoverability and provide important information to users.♻️ Recommended additions
{ "name": "@skeletor/darwin-x64", "version": "0.3.10", "description": "darwin-x64 binary for skeletor", + "license": "MIT", + "repository": { + "type": "git", + "url": "https://github.com/theGeekist/skeletor.git" + }, + "author": "theGeekist", + "homepage": "https://github.com/theGeekist/skeletor#readme", "os": [ "darwin" ], "cpu": [ "x64" ], "files": [ "bin" ] }.github/workflows/release.yml (4)
146-156: Consider simplifying Windows binary handling.The current approach copies the binary as
skeletorand then renames it toskeletor.exe. This could be streamlined by copying directly with the correct name.♻️ Proposed simplification
set -euo pipefail mkdir -p "dist/${{ matrix.out }}" # Handle potential .exe extension for Windows BIN="target/${{ matrix.target }}/release/skeletor" + OUT_NAME="skeletor" if [[ "${{ matrix.os }}" == "windows-latest" ]]; then BIN="${BIN}.exe" + OUT_NAME="skeletor.exe" fi - cp "$BIN" "dist/${{ matrix.out }}/skeletor" - if [[ "${{ matrix.os }}" == "windows-latest" ]]; then - mv "dist/${{ matrix.out }}/skeletor" "dist/${{ matrix.out }}/skeletor.exe" - fi + cp "$BIN" "dist/${{ matrix.out }}/${OUT_NAME}" tar -C dist -czf "${{ matrix.out }}.tar.gz" "${{ matrix.out }}"
180-180: Condition is now always true.Since all matrix entries now include the
npm_platformfield, the conditionif: ${{ matrix.npm_platform != '' }}will always evaluate to true. Consider removing this condition or documenting the intent if you plan to add matrix entries without npm support in the future.
184-198: Refactor duplicated Windows binary handling.The Windows
.exehandling logic (lines 191-198) is duplicated from the tarball packaging step (lines 148-156). Consider extracting this to a reusable script or simplifying the copy operation.♻️ Proposed simplification
set -euo pipefail # Move binary to the correct location PKG_DIR="npm/platforms/${{ matrix.npm_platform }}" mkdir -p "$PKG_DIR/bin" BIN="target/${{ matrix.target }}/release/skeletor" - if [[ "${{ matrix.os }}" == "windows-latest" ]]; then + OUT_NAME="skeletor" + if [[ "${{ matrix.os }}" == "windows-latest" ]]; then BIN="${BIN}.exe" + OUT_NAME="skeletor.exe" fi - cp "$BIN" "$PKG_DIR/bin/skeletor" - - if [[ "${{ matrix.os }}" == "windows-latest" ]]; then - mv "$PKG_DIR/bin/skeletor" "$PKG_DIR/bin/skeletor.exe" - fi + cp "$BIN" "$PKG_DIR/bin/${OUT_NAME}" # Publish
257-262: Verify platform-specific packages were published before publishing main package.The main package depends on platform-specific packages as optional dependencies, but there's no verification that all platform packages were successfully published before the main package is published. A failure in one platform's publish step could result in the main package being published with missing platform binaries.
Consider adding a verification step or making the publish-npm job depend on all platform publish steps completing successfully. Alternatively, the current design where both publish in parallel may be acceptable if npm handles missing optional dependencies gracefully, but this should be documented.
npm/platforms/linux-x64/package.json (1)
1-14: Add standard npm package metadata fields.Similar to the darwin-x64 package, this manifest is missing common npm package fields such as
license,repository,author, andhomepage. All platform-specific packages should have consistent metadata.♻️ Recommended additions
{ "name": "@skeletor/linux-x64", "version": "0.3.10", "description": "linux-x64 binary for skeletor", + "license": "MIT", + "repository": { + "type": "git", + "url": "https://github.com/theGeekist/skeletor.git" + }, + "author": "theGeekist", + "homepage": "https://github.com/theGeekist/skeletor#readme", "os": [ "linux" ], "cpu": [ "x64" ], "files": [ "bin" ] }npm/skeletor/package.json (1)
3-3: Consider automating version synchronization.The version
0.3.10is duplicated in 7 places. During releases, ensure the CI workflow updates all occurrences atomically. A pre-publish script or workspace tool (e.g.,changeset,lerna) could help prevent version drift.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 66ebb837f2af12e1917d1901ee642b236c889208 and 2d6d95341b22a1a29b8333e208f6a435e559dfef.
📒 Files selected for processing (15)
.github/workflows/release.yml.gitignoreREADME.mdinstall.shnpm/platforms/darwin-arm64/bin/skeletornpm/platforms/darwin-arm64/package.jsonnpm/platforms/darwin-x64/package.jsonnpm/platforms/linux-arm64/package.jsonnpm/platforms/linux-x64/package.jsonnpm/platforms/win32-arm64/package.jsonnpm/platforms/win32-x64/package.jsonnpm/skeletor/bin/skeletornpm/skeletor/package.jsontest_output/hello.rstest_output/module.rs
⏰ 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). (1)
- GitHub Check: test
🔇 Additional comments (13)
.gitignore (1)
39-39: Verify the intentionality of narrowing the test_output ignore pattern.The change narrows the ignore pattern from a broad
test_output/directory to a specific pathtest_output/npm/platforms/*/bin/. This means other files undertest_output/will now be tracked by Git, which could lead to unintended test artifacts being committed to the repository.Please confirm:
- Is this narrowing intentional?
- Should other test output files under
test_output/also be ignored?- If the intent is to only ignore binaries, would a broader pattern like
test_output/npm/be more appropriate to prevent any test artifacts from leaking into version control?npm/skeletor/bin/skeletor (2)
1-6: LGTM!The shebang and module imports are correct and appropriate for this CLI wrapper.
7-14: LGTM!The platform mapping is comprehensive and covers all major architectures for macOS, Linux, and Windows.
.github/workflows/release.yml (2)
106-126: LGTM!The npm platform entries in the build matrix are comprehensive and align with the platform-specific package definitions.
173-178: LGTM!The Node.js setup is configured correctly with an appropriate version and registry URL.
README.md (1)
86-120: LGTM!The installation documentation is well-organized with consistent badge styling. The npm installation instructions are clear and provide both global install and npx usage patterns, which is helpful for users.
npm/skeletor/package.json (2)
19-29: LGTM!Repository metadata, author, license, bugs, and homepage fields are correctly configured.
5-7: Bin script is properly configured.The
bin/skeletorfile exists and has the correct shebang (#!/usr/bin/env node).install.sh (5)
47-53: Windows support relies on bash-compatible environments.This script runs under MSYS2, Cygwin, or MinGW, which is appropriate for a bash script. Users running PowerShell or cmd.exe natively won't be able to use this installer.
Consider documenting this in the README or providing a separate
install.ps1for native Windows users.
100-123: LGTM!The binary extraction logic with fallback
findis robust. The debug output on failure will help users diagnose archive structure issues.
150-157: LGTM!Clear success message and helpful PATH warning for users installing to
~/.local/bin.
14-24: LGTM!The dual-approach version resolution (API first, redirect fallback) handles rate-limited scenarios gracefully.
78-88: No changes needed. The checksum file format in your releases contains only the hash without a filename, so the current implementation correctly reads the entire file content intoEXPECTED_SUM.Likely an incorrect or invalid review comment.
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. ℹ️ You can also turn on project coverage checks and project coverage reporting on Pull Request comment Thanks for integrating Codecov - We've got you covered ☂️ |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @install.sh:
- Line 76: The curl invocation that downloads the checksum file (using
CHECKSUM_URL -> TMP_DIR/CHECKSUM_ASSET in install.sh) lacks a pre-check for the
file's existence; add a conditional that first verifies the checksum URL is
reachable (e.g., with a HEAD/fail check or curl --head) and only runs the curl
download if that check succeeds, otherwise skip checksum download and emit a
clear warning/log; ensure you reference CHECKSUM_URL, TMP_DIR, and
CHECKSUM_ASSET and preserve exit-on-error behavior for real failures while
allowing missing checksum files (older releases) to continue installation.
- Around line 127-148: The else branch contains an unreachable check that tests
[[ ! -w "${INSTALL_DIR}" ]] and sets USE_SUDO=true even though that else only
runs when INSTALL_DIR is writable; remove that inner if-block (lines referencing
INSTALL_DIR and USE_SUDO) so the logic simply treats the writable non-root case
as not requiring sudo, or alternatively move the sudo check into the non-root
branch and set USE_SUDO=true when /usr/local/bin exists but is not writable;
update the code paths around INSTALL_DIR and USE_SUDO accordingly.
In @npm/skeletor/package.json:
- Line 20: Update the "url" field in package.json to use the repository owner's
canonical casing by changing "git+https://github.com/thegeekist/skeletor.git" to
"git+https://github.com/theGeekist/skeletor.git" so it matches other files
(install.sh, README.md); locate the "url" property in package.json and replace
the owner segment casing only, leaving the rest of the URL unchanged.
🧹 Nitpick comments (1)
npm/skeletor/package.json (1)
16-17: Consider removing empty scripts object.The empty
scriptsobject can be removed if no npm scripts are needed, making the manifest cleaner.🧹 Optional cleanup
}, - "scripts": { - }, "repository": {
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 2d6d95341b22a1a29b8333e208f6a435e559dfef and 8a077b47a1ccabcd3f29de0dc840d75c36b0efb4.
📒 Files selected for processing (11)
.github/workflows/release.ymlREADME.mdinstall.shnpm/platforms/darwin-arm64/package.jsonnpm/platforms/darwin-x64/package.jsonnpm/platforms/linux-arm64/package.jsonnpm/platforms/linux-x64/package.jsonnpm/platforms/win32-arm64/package.jsonnpm/platforms/win32-x64/package.jsonnpm/skeletor/bin/skeletornpm/skeletor/package.json
🚧 Files skipped from review as they are similar to previous changes (8)
- npm/platforms/linux-x64/package.json
- .github/workflows/release.yml
- npm/platforms/win32-arm64/package.json
- npm/platforms/darwin-arm64/package.json
- npm/platforms/linux-arm64/package.json
- npm/platforms/win32-x64/package.json
- npm/skeletor/bin/skeletor
- npm/platforms/darwin-x64/package.json
⏰ 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). (1)
- GitHub Check: test
🔇 Additional comments (4)
install.sh (3)
100-123: LGTM! Robust binary extraction with proper fallback.The Windows binary name handling and fallback search logic are well-implemented. The error message with directory listing (line 121) is particularly helpful for debugging.
47-54: LGTM! Windows detection covers common environments.The pattern matching for MSYS, Cygwin, and MinGW environments is appropriate, and the MSVC target triples are correct.
42-45: LGTM! Linux ARM64 support added correctly.The architecture mapping follows the established pattern and uses the correct target triple.
README.md (1)
86-120: LGTM! Clear installation documentation.The NPM installation instructions align well with the new npm packaging support. The reordered options with badges improve readability, and the commands follow npm best practices.
This PR adds support for distributing
skeletoras an npm package.Changes
npm/skeletormain package and platform wrappers..github/workflows/release.ymlto publish to npm.install.shwith Linux ARM64 support, Windows support, and checksum verification.README.mdwith npm installation instructions and verified badges.Notes
NPM_TOKENsecret in GitHub.Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.