Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add electron-rebuild to suite test #3392

Merged
merged 12 commits into from
Mar 27, 2024
Merged

Conversation

bugsounet
Copy link
Contributor

@bugsounet bugsounet commented Mar 2, 2024

because actually i'm not able to rebuild any libraries to works with electron v29.x
I write a suite test to check electron-rebuild

Note: works with v28.x

@bugsounet bugsounet marked this pull request as draft March 2, 2024 11:03
@bugsounet
Copy link
Contributor Author

force to use electron v28 result:

Run npx electron-rebuild
- Searching dependency tree
✔ Rebuild Complete

electron v29 failed

@bugsounet bugsounet marked this pull request as ready for review March 2, 2024 13:11
Copy link
Collaborator

@rejas rejas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @bugsounet so what do we get with this new test? what benefit do we have? just trying to understand

@bugsounet
Copy link
Contributor Author

Some complex modules use electron rebuild
perhaps in line with a new electron version, it might be important to know if our development version works before publishing the release
I don't... it's the job of 3rd party module
but it can help them

It's up to you if you want to add it or not.
In my case I already have a test suite to see if it is compatible or not with the master branch of MM²

@sdetweil
Copy link
Collaborator

many modules use libraries which have binary components tied to node and electron versions

our update causes them to fail, they need to rebuild.

we should ship a compatible version of electron rebuild for this purpose. it must be located in the same node_modules folder as the electron install now

@khassel
Copy link
Collaborator

khassel commented Mar 22, 2024

electron v29 failed

node@80ef79387f2d:/opt/magic_mirror$ python3 --version
Python 3.11.2
node@80ef79387f2d:/opt/magic_mirror$ cat /etc/os-release
PRETTY_NAME="Debian GNU/Linux 12 (bookworm)"
NAME="Debian GNU/Linux"
VERSION_ID="12"
VERSION="12 (bookworm)"
VERSION_CODENAME=bookworm
ID=debian
HOME_URL="https://www.debian.org/"
SUPPORT_URL="https://www.debian.org/support"
BUG_REPORT_URL="https://bugs.debian.org/"
node@80ef79387f2d:/opt/magic_mirror$ npx electron --version
v29.1.5
node@80ef79387f2d:/opt/magic_mirror$ npx electron-rebuild --version
Electron Rebuild Version: 3.6.0
node@80ef79387f2d:/opt/magic_mirror$ node -v
v21.7.1
node@80ef79387f2d:/opt/magic_mirror$ npx electron-rebuild
✔ Rebuild Complete

@bugsounet
Copy link
Contributor Author

@khassel: yes it's solved since last week ;)

@khassel
Copy link
Collaborator

khassel commented Mar 22, 2024

Hi @bugsounet so what do we get with this new test? what benefit do we have? just trying to understand

@rejas this should be merged. we need such a test to see if electron-rebuild fails if we update electron

@bugsounet
Copy link
Contributor Author

After, I don't know if we can do this test only for information

If test failed -> just warn but we can merge

@sdetweil
Copy link
Collaborator

is electron/rebuild in package.json for production?

@bugsounet
Copy link
Contributor Author

is electron/rebuild in package.json for production?

@sdetweil : Actually no, but you want to add it ?
@rejas, @khassel : what do you think ?

@khassel
Copy link
Collaborator

khassel commented Mar 23, 2024

@rejas, @khassel : what do you think ?

I like the idea to test electron-rebuild against the electron version but I don't want to have electron-rebuild in package.json.

I think only a minority of users are using such hardware related modules so we should not install this for all. Next point is, that electron-rebuild needs python3 and build-essential rpm's to work, but these are not installed on every os per default, e.g. not in the docker images (which would increase size of them massively). So we should stay with current approach where these modules must install electron-rebuild if needed.

@bugsounet
Copy link
Contributor Author

@khassel: that why i install it only for testing in test-suite ;)

@khassel khassel requested a review from rejas March 27, 2024 20:43
@rejas rejas merged commit be63e36 into MagicMirrorOrg:develop Mar 27, 2024
6 checks passed
@bugsounet bugsounet deleted the rebuild branch March 28, 2024 06:26
@rejas rejas mentioned this pull request Apr 1, 2024
rejas added a commit that referenced this pull request Apr 1, 2024
## [2.27.0] - 2024-04-01

Thanks to: @bugsounet, @crazyscot, @illimarkangur, @jkriegshauser,
@khassel, @KristjanESPERANTO, @Paranoid93, @rejas, @sdetweil and
@vppencilsharpener.

This release marks the first release without Michael Teeuw (@MichMich).
A very special thanks to him for creating MagicMirror and leading the
project for so many years.

For more info, please read the following post: [A New Chapter for
MagicMirror: The Community Takes the
Lead](https://forum.magicmirror.builders/topic/18329/a-new-chapter-for-magicmirror-the-community-takes-the-lead).

### Added

- Output of system information to the console for troubleshooting (#3328
and #3337), ignore errors under aarch64 (#3349)
- [chore] Add `eslint-plugin-package-json` to lint the `package.json`
files (#3368)
- [weather] `showHumidity` config is now a string describing where to
show this element. Supported values: "wind", "temp", "feelslike",
"below", "none". (#3330)
- electron-rebuild test suite for electron and 3rd party modules
compatibility (#3392)
- Create MM² icon and attach it to electron process (#3407)

### Updated

- Update updatenotification (update_helper.js): Recode with pm2 library
(#3332)
- Removing lodash dependency by replacing merge by spread operator
(#3339)
- Use node prefix for build-in modules (#3340)
- Rework logging colors (#3350)
- Update pm2 to v5.3.1 with no allow-ghsas (#3364)
- [chore] Update husky and let lint-staged fix ESLint issues
- [chore] Update dependencies including electron to v29 (#3357) and
node-ical
- Update translations for estonian (#3371)
- Update electron to v29 and update other dependencies
- [calendar] fullDay events over several days now show the left days
from the first day on and 'today' on the last day
- Update layout of current weather indoor values

### Fixed

- Correct apibase of weathergov weatherprovider to match documentation
(#2926)
- Worked around several issues in the RRULE library that were causing
deleted calender events to still show, some
initial and recurring events to not show, and some event times to be off
an hour. (#3291)
- Skip changelog requirement when running tests for dependency updates
(#3320)
- Display precipitation probability when it is 0% instead of blank/empty
(#3345)
- [newsfeed] Suppress unsightly animation cases when there are 0 or 1
active news items (#3336)
- [newsfeed] Always compute the feed item URL using the same helper
function (#3336)
- Ignore all custom css files (#3359)
- [newsfeed] Fix newsfeed stall issue introduced by #3336 (#3361)
- Changed `log.debug` to `log.log` in `app.js` where logLevel is not set
because config is not loaded at this time (#3353)
- [calendar] deny fetch interval < 60000 and set 60000 in this case
(prevent fetch loop failed) (#3382)
- added message in case where config.js is missing the module.export
line PR #3383
- Fixed an issue where recurring events could extend past their
recurrence end date (#3393)
- Don't display any `npm WARN <....>` on install (#3399)
- Fixed move suncalc dependency to production from dev, as it is used by
clock module
- [compliments] Fix mirror not responding anymore when no compliments
are to be shown (#3385)

### Deleted

- Unneeded file headers (#3358)

---------

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: Michael Teeuw <michael@xonaymedia.nl>
Co-authored-by: Kristjan ESPERANTO <35647502+KristjanESPERANTO@users.noreply.github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Karsten Hassel <hassel@gmx.de>
Co-authored-by: Ross Younger <crazyscot@gmail.com>
Co-authored-by: Bugsounet - Cédric <github@bugsounet.fr>
Co-authored-by: jkriegshauser <joshuakr@nvidia.com>
Co-authored-by: illimarkangur <116028111+illimarkangur@users.noreply.github.com>
Co-authored-by: sam detweiler <sdetweil@gmail.com>
Co-authored-by: vppencilsharpener <tim.pray@gmail.com>
Co-authored-by: Paranoid93 <6515818+Paranoid93@users.noreply.github.com>
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.

5 participants