Skip to content

Conversation

@Pertempto
Copy link
Contributor

  • Add example config
  • Improve how we handle modules

@github-actions
Copy link

github-actions bot commented Nov 7, 2025

scripts/install.sh: Use of npm ci may fail for many third-party modules because npm ci requires a package-lock.json (or shrinkwrap). Right now the script only runs npm ci when package.json exists — this will error for modules without a lockfile and cause missed installs. Suggest changing to try npm ci then fall back to npm install --omit=dev (or just use npm install --omit=dev). Also increment MODULE_COUNT for successfully cloned modules (not only when package.json exists) so the reported count matches what's installed.

@github-actions
Copy link

github-actions bot commented Nov 7, 2025

Changes Requested

Please address the following changes before I re-review:

  • Make module installs robust: remove partial module dirs when git clone fails; increment MODULE_COUNT on successful clone (not only after deps); and install dependencies conditionally — prefer npm ci --omit=dev when package-lock.json exists, otherwise use npm install --omit=dev.
  • Make adding the user-local npm PATH idempotent and reference the resolved user home: only append the export PATH line to "$MM_HOME/.bashrc" if it does not already exist, write the expanded $MM_HOME (not $HOME), and preserve ownership.
  • Ensure PM2-launched processes can find user-local global packages: include the user-local npm bin in the env.PATH written to ecosystem.config.js (expand $MM_HOME when writing the file), and chown the file to the target user.
  • Use a preserved HOME when running user-scoped commands: prefer sudo -H -u "$MM_USER" ... or ensure commands executed via bash -lc reference $MM_HOME rather than $HOME.

Summary of Changes

  • Added example configuration files under example/ and a project README.md.
  • Replaced installation.md with install.md and documented usage of scripts/install.sh.
  • Updated scripts/install.sh to use modules.json, added jq to prerequisites, shallow-cloned MagicMirror, configured a user-local npm prefix for PM2, and added a new modules install loop that clones modules from modules.json and attempts to install their dependencies.
  • Switched to user-local PM2 install and added several debug checks and PATH exports.

Overall Feedback

  • Negative: The new module-install and PM2 handling introduce correctness/idempotency issues that will cause confusing failures:

    • npm ci is used unconditionally; it fails without a lockfile and will abort module installs.
    • Partial module directories can remain when git clone fails, and MODULE_COUNT only increments after dependency install so it underreports.
    • The PM2 env.PATH currently omits the user-local npm bin; PM2-launched processes may not find user-installed global packages.
  • Positive: The move to modules.json with jq, shallow cloning MagicMirror, and using a user-local npm prefix for PM2 are solid improvements. Scoping pm2/npm commands to the target user and the extra diagnostics make debugging easier. With the requested fixes the installer will be robust and idempotent — great progress!

Please address the requested fixes (I left an inline actionable comment on the module install loop) and ping me when updated. Thanks! @Pertempto

@kwila-cloud kwila-cloud deleted a comment from github-actions bot Nov 7, 2025
@kwila-cloud kwila-cloud deleted a comment from github-actions bot Nov 7, 2025
@github-actions
Copy link

github-actions bot commented Nov 7, 2025

scripts/install.sh: In the ecosystem.config.js block the env.PATH is set to '/usr/local/bin:/usr/bin:/bin'. Please include the user-local npm bin so PM2-launched processes have the same runtime PATH as the target user. Suggestion:

Replace the PATH line with something like:

PATH: '$MM_HOME/.npm-global/bin:/usr/local/bin:/usr/bin:/bin',

This prevents "command not found" for user-local global packages and matches the rest of the script's npm prefix usage.

@github-actions
Copy link

github-actions bot commented Nov 7, 2025

In scripts/install.sh consider ensuring commands that rely on the target user's HOME use a preserved HOME. Use either sudo -H -u "$MM_USER" ... or explicitly reference "$MM_HOME" inside the command string.

Example alternatives:

  • sudo -H -u "$MM_USER" npm config set prefix "$MM_HOME/.npm-global"
  • sudo -u "$MM_USER" bash -lc "export PATH='$MM_HOME/.npm-global/bin:\$PATH' && pm2 --version"

This avoids relying on the invoking environment's $HOME and makes behavior consistent across systems/sudo configurations.

@github-actions
Copy link

github-actions bot commented Nov 7, 2025

@Pertempto — Module install loop: after git clone fails you log a warning but leave any partially created dir behind and only increment MODULE_COUNT when a package.json exists and deps installed. Please:

  • Remove a module dir if git clone fails (or detect clone success before proceeding).
  • Increment MODULE_COUNT on successful clone (not only after deps) so the reported count reflects installed modules.

Relevant block: scripts/install.sh lines ~235-244.

@github-actions
Copy link

github-actions bot commented Nov 7, 2025

@Pertempto �1: ecosystem.config.js PATH: please include the user-local npm bin so PM2-launched processes can find user-installed global packages. Suggest replacing the PATH with:

PATH: '$MM_HOME/.npm-global/bin:/usr/local/bin:/usr/bin:/bin:/usr/local/sbin:/usr/sbin:/sbin',

This file is written at scripts/install.sh lines ~296-307.

@github-actions
Copy link

github-actions bot commented Nov 8, 2025

scripts/install.sh — adding user-local npm PATH to ~/.bashrc (lines ~261-264):

  • Issue: Current append uses "$HOME" and always writes without checking for existing entry earlier in the script run; you do check with grep -q which is good, but the line uses $HOME which for sudo may not be target user's home when executed as root.
  • Actionable change: write the export using the resolved MM_HOME and ensure it's created as the target user (ownership). Example replacement:
if ! grep -q ".npm-global/bin" "$MM_HOME/.bashrc"; then
  echo "export PATH=\"$MM_HOME/.npm-global/bin:\$PATH\"" >> "$MM_HOME/.bashrc"
  chown "$MM_USER:$MM_USER" "$MM_HOME/.bashrc"
fi

This ensures the PATH line references the real user home and preserves file ownership. Please update accordingly. @Pertempto

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