Skip to content

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

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

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

Conversation

@angeldeejay

@angeldeejay angeldeejay commented May 5, 2026

Copy link
Copy Markdown
Contributor

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

angeldeejay commented May 5, 2026

Copy link
Copy Markdown
Contributor Author

@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

khassel commented May 5, 2026

Copy link
Copy Markdown
Collaborator

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

sdetweil commented May 5, 2026

Copy link
Copy Markdown
Collaborator

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

@khassel

khassel commented May 5, 2026

Copy link
Copy Markdown
Collaborator

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

angeldeejay commented May 5, 2026

Copy link
Copy Markdown
Contributor Author

@khassel done!

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

angeldeejay commented May 5, 2026

Copy link
Copy Markdown
Contributor Author

@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
@rejas rejas mentioned this pull request Jul 1, 2026
rejas added a commit that referenced this pull request Jul 1, 2026
## Release Notes
Thanks to: @angeldeejay, @egeekial, @khassel, @KristjanESPERANTO,
@MikeBishop, @rejas
> ⚠️ This release needs nodejs version >=22.21.1 <23 || >=24 (no change
to previous release)

[Compare to previous Release
v2.36.0](v2.36.0...develop)


### [core]
- Prepare Release 2.37.0 (#4193)
- fix(electron): map IPv6 :: wildcard to localhost (#4188)
- refactor(main): modernize DOM update flow with async/await (#4186)
- refactor(main): simplify _updateDom with async/await (#4185)
- fix(security): prevent unauthorized secret expansion in socket
payloads (#4184)
- refactor(main): simplify updateDomWithContent async flow (#4182)
- fix: modules losing data after HTTP 304 responses (#4180)
- chore: add missing core defaults (#4181)
- fix(server): enforce ipWhitelist for Socket.IO too (#4169)
- feat(systeminfo): include Git hash and branch in system information
log (#4167)
- feat(electron): support object-based electronSwitches (#4161)
- systeminformation thread not ending: move error handling from utils to
app (#4160)
- fix systeminformation thread not ending (#4155)
- refactor: use ES module imports in browser core (#4158)
- refactor(core): remove old Object.assign polyfill (#4157)
- refactor: rewrite Module as an ES6 class (#4151)
- refactor: rewrite NodeHelper as an ES6 class (#4147)
- update eletron to v42 (#4144)
- refactor(utils): drop ajv dependency (#4142)
- fix(systeminformation): output right 'used node' version (from parent
process) (#4141)
- fix: skip postinstall git clean when not in a git repository (#4139)
- Remove unnecessary conditionals and fix falsy property check in
imperial conversion (#4135)
- update version in package.json

### [dependencies]
- update dependencies (#4191)
- Bump actions/checkout from 6 to 7 (#4190)
- chore: update dependencies and adjust import path for SunCalc (#4189)
- update dependencies incl. electron and revert
yauzl-electron-install-fix (#4183)
- update dependencies, add electron fix in package.json (#4175)
- chore: update dependencies (#4162)
- Bump actions/dependency-review-action from 4 to 5 (#4152)
- Unify linting: replace Stylelint and markdownlint with ESLint (#4148)
- update dependencies and workflows to node v26 (#4140)

### [modules/alert]
- CodeQL cleanup for alerts #18, #19, #20 (#4153)
- fix: resolve CodeQL alerts #24 and #26 (#4145)
- fix(electron): resolve CodeQL alerts #22 and #25 in electron.js
(#4136)

### [modules/calendar]
- perf(calendar): pre-filter ICS data before parsing (#4168)
- perf(calendar): use async ICS parsing to avoid blocking event loop
(#4143)

### [modules/newsfeed]
- [newsfeed] add allowBasicHtmlTags option for basic emphasis (#4176)

### [modules/updatenotification]
- fix(updatenotification): don't spawn a child process when running
under PM2 (#4166)
- fix(updatenotification): use process.argv[0] as restart binary (#4163)
- fix(updatenotification): preserve start mode on restart (#4156)
- fix(updatenotification): fix ref diff parsing for fetch --dry-run
(#4138)
- refactor(updatenotification): replace pm2 usage with node logic
(#4134)

### [modules/weather]
- feat(weather): add Buienradar provider (#4164)

### [testing]
- remove warning in unit tests (for nodejs >= v25) (#4149)
- polish HTTP 304 docs/test/handling (#4129)

---------

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: Kristjan ESPERANTO <35647502+KristjanESPERANTO@users.noreply.github.com>
Co-authored-by: BugHaver <43462320+bughaver@users.noreply.github.com>
Co-authored-by: BugHaver <43462320+lsaadeh@users.noreply.github.com>
Co-authored-by: Karsten Hassel <hassel@gmx.de>
Co-authored-by: Magnus <34011212+MagMar94@users.noreply.github.com>
Co-authored-by: Koen Konst <koenspero@gmail.com>
Co-authored-by: Koen Konst <c.h.konst@avisi.nl>
Co-authored-by: Bugsounet - Cédric <github@bugsounet.fr>
Co-authored-by: dathbe <github@beffa.us>
Co-authored-by: veeck <gitkraken@veeck.de>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Marcel <m-idler@users.noreply.github.com>
Co-authored-by: sam detweiler <sdetweil@gmail.com>
Co-authored-by: Kevin G. <crazylegstoo@gmail.com>
Co-authored-by: Jboucly <33218155+jboucly@users.noreply.github.com>
Co-authored-by: Jboucly <contact@jboucly.fr>
Co-authored-by: Jarno <54169345+jarnoml@users.noreply.github.com>
Co-authored-by: Jordan Welch <JordanHWelch@gmail.com>
Co-authored-by: Blackspirits <blackspirits@gmail.com>
Co-authored-by: Samed Ozdemir <samed@xsor.io>
Co-authored-by: in-voker <58696565+in-voker@users.noreply.github.com>
Co-authored-by: Andrés Vanegas Jiménez <142350+angeldeejay@users.noreply.github.com>
Co-authored-by: cgillinger <christian.gillinger@gmail.com>
Co-authored-by: Sonny B <43247590+sonnyb9@users.noreply.github.com>
Co-authored-by: sonnyb9 <sonnyb9@users.noreply.github.com>
Co-authored-by: Morgan McBee <egeekial@users.noreply.github.com>
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
Co-authored-by: Mike Bishop <mbishop@evequefou.be>
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