Skip to content

fix: skip postinstall git clean when not in a git repository#4139

Merged
khassel merged 1 commit intoMagicMirrorOrg:developfrom
angeldeejay:develop
May 5, 2026
Merged

fix: skip postinstall git clean when not in a git repository#4139
khassel merged 1 commit intoMagicMirrorOrg:developfrom
angeldeejay:develop

Conversation

@angeldeejay
Copy link
Copy Markdown
Contributor

@angeldeejay angeldeejay commented May 5, 2026

What does this PR accomplish?

The postinstall script runs git clean -df fonts vendor modules/default
unconditionally. When magicmirror is installed as an npm dependency
(e.g. npm install magicmirror or as a transitive dep in another project),
npm runs postinstall in a directory that is not a git repository.
This causes:

fatal: not a git repository (or any of the parent directories): .git

npm treats the non-zero exit as a failure → the entire installation aborts
with code 128. This makes it impossible to use magicmirror as an npm
dependency in any third-party project.

The issue has been present since at least v2.34.0. In v2.35.0
modules/default was added to the clean targets, but the root cause
predates that change.

Fix

Added scripts/postinstall.js — a cross-platform Node.js script that
guards the git clean call with a git rev-parse --git-dir check.

  • When running inside a real git repository: behaviour is identical to before.
  • When running outside one (e.g. installed as an npm package): step is silently skipped.

package.json postinstall updated from the bare git clean call to:

"postinstall": "node scripts/postinstall.js"

This approach is cross-platform (Linux, macOS, Windows) and keeps the
logic readable rather than inlined as a one-liner.

Does this solve a related issue?

No existing issue tracked. Discovered while using magicmirror as a
devDependency in a companion tooling project — installation failed
unconditionally on all platforms.

Checklist

  • Targets the develop branch
  • No visual changes (script + package.json only)
  • node --run lint:prettier run — no changes needed for .js script

@angeldeejay
Copy link
Copy Markdown
Contributor Author

angeldeejay commented May 5, 2026

@KristjanESPERANTO can you take a look on this please? I've found this issue while developing MagicMirror sandbox

@KristjanESPERANTO
Copy link
Copy Markdown
Collaborator

Looks good to me. But I'm not the only maintainer 🙂

@khassel, what do you think? Maybe we should take this opportunity to add a note to the script explaining that this is necessary for migrating from an older systems (<2.35.0) to a newer one, and that we can remove it in a few years. We wouldn't have been able to include a comment like that in package.json.

@khassel
Copy link
Copy Markdown
Collaborator

khassel commented May 5, 2026

As far as I'm concerned, it can be merged. Another option would be to remove the postinstall command entirely.

Its sole purpose is to remove folders whose contents have been moved and which would otherwise appear in git status.

For installations that haven't been migrated yet, that's just how it is; the user will have to clean it up themselves.

@sdetweil
Copy link
Copy Markdown
Collaborator

sdetweil commented May 5, 2026

I will update the update script to remove the old default module folder

@khassel
Copy link
Copy Markdown
Collaborator

khassel commented May 5, 2026

I will update the update script to remove the old default module folder

with this I think the best choice is to remove the postinstall command entirely.

@angeldeejay could you please update this PR accordingly? Thanks and sorry for the inconvenience...

@angeldeejay
Copy link
Copy Markdown
Contributor Author

angeldeejay commented May 5, 2026

@khassel done!

@angeldeejay angeldeejay force-pushed the develop branch 2 times, most recently from 8801b44 to 810d852 Compare May 5, 2026 19:43
@angeldeejay
Copy link
Copy Markdown
Contributor Author

angeldeejay commented May 5, 2026

@khassel @KristjanESPERANTO Do you want me to drop lock files changes and leave only package.json ones?

@angeldeejay
Copy link
Copy Markdown
Contributor Author

Assumed yes. I'm guessing your pipelines update lock files by themselves

@khassel khassel merged commit b474198 into MagicMirrorOrg:develop May 5, 2026
12 checks passed
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.

4 participants