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

Electron 20 causes rebuild failures in epoll (used by all the GPIO modules, pir, etc) #2903

Closed
sdetweil opened this issue Sep 4, 2022 · 18 comments

Comments

@sdetweil
Copy link
Collaborator

sdetweil commented Sep 4, 2022

to recreate
install MM
switch to develop
install MMM-PIR-Sensor

boom..

I opened this issue on epoll, which needs to drop nan and use node-addon-api
but that means a near total rewrite.. SO.. its unlikely any time soon

(fivdi/epoll#47, closed as dup of 42)
was already on the list, as enhancement from a year ago
fivdi/epoll#42

here is the electron issue
electron/electron#18397

we cannot go above electron 19 at this time

@sdetweil
Copy link
Collaborator Author

sdetweil commented Sep 4, 2022

I see that MMM-PIR-Sensor-Lite uses python to get signals.. and installs ok as it doesn't use epoll
https://github.com/grenagit/MMM-PIR-Sensor-Lite

the major PIR modules all appear archived..

@bugsounet
Copy link
Contributor

bugsounet commented Sep 4, 2022

Hi, it's related to this MM topic

in my case not working with V20.x:

EXT-Pir: use epool library
EXT-Detector: use node snowboy library
Gateway: use node-pty
EXT-MusicPlayer : use node usb

Naturaly, Other old modules (from other user) have same issue

@sdetweil
Copy link
Collaborator Author

sdetweil commented Sep 4, 2022

snowboy uses nan

latest node usb should be ok, removed nan

node-pty uses nan

epoll uses nan

@bugsounet
Copy link
Contributor

Perfect thx @khassel ;)

@khassel
Copy link
Collaborator

khassel commented Dec 2, 2022

any progress here?

Because:

End of Support for 19.x.y

Electron 19.x.y has reached end-of-support as per the project's support policy. Developers and applications are encouraged to upgrade to a newer version of Electron.

Quoted from https://releases.electronjs.org/releases/stable

@bugsounet
Copy link
Contributor

If for you it's really important... change it!
My opinion does not count, I am not an official MagicMirror developer (or modules developer)

But do not warn at the last moment!
I think that an special annonce in forum is needed

@bugsounet

@bugsounet
Copy link
Contributor

bugsounet commented Dec 3, 2022

Infos:

Tested with electron v22.0.0

✔ MagicMirror Rebuild Complete

So should be ok (Most of sensible library)

@khassel
Copy link
Collaborator

khassel commented Dec 3, 2022

Thanks!

So I will update to electron v22 before next release.

@rejas
Copy link
Collaborator

rejas commented Dec 4, 2022

But is the original issue solved (electron rebuild failure) with electron v22?

And will the electron22 requirements be stricter again (fewer architectures supported for instance)?

@sdetweil
Copy link
Collaborator Author

sdetweil commented Dec 4, 2022

he has his own rebuild. we should ship electron-rebuild.

a secondary problem is that it needs to be installed in the mm node_modules folder. many apps install it locally in their module folder but that doesn't work anymore.

@bugsounet
Copy link
Contributor

bugsounet commented Dec 4, 2022

he has his own rebuild. we should ship electron-rebuild.

it's +/- the same but simplified for MM²

a secondary problem is that it needs to be installed in the mm node_modules

I never do this

many apps install it locally in their module folder but that doesn't work anymore.

with electron-rebuild:
~/MagicMirror/modules/EXT-Pir$ ./node_modules/.bin/electron-rebuild
⠧ Building module: epoll, Completed: 0make: Entering directory '/home/bugsounet/MagicMirror/modules/EXT-Pir/node_modules/epoll/build'
CXX(target) Release/obj.target/epoll/src/epoll.o
⠴ Building module: epoll, Completed: 0 SOLINK_MODULE(target) Release/obj.target/epoll.node
⠦ Building module: epoll, Completed: 0 COPY Release/epoll.node
make: Leaving directory '/home/bugsounet/MagicMirror/modules/EXT-Pir/node_modules/epoll/build'
✔ Rebuild Complete

~/MagicMirror$ cat package.json | grep '"electron":'
                "electron": "^22.0.0"
~/MagicMirror$ cat package-lock.json | grep '"electron":'
                                "electron": "^22.0.0"
~/MagicMirror$ git branch
* develop

@sdetweil
Copy link
Collaborator Author

sdetweil commented Dec 4, 2022

@bugsounet that is because electron-rebuild is installed in the module folder..

if u install it in MM folder and then reference there from module folder it works

../../node_modules/.bin/electron-rebuild 

I have fixed a bunch of modules by making a postinstall script that does that.. vs hard coding electron-rebuild in package.json

for example here
https://github.com/sdetweil/MMM-Buttons

@bugsounet
Copy link
Contributor

Are you really sure that install electron-rebuild in MM folder is a good choice ?
I'm really not sure.
You will make issue for updating MM²
Most of users are lost with errors on updating

But it's your choice, make as you want ;)
Developers have to done the better to catch any errors (and don't have a black screen)

Best way is installing electron rebuild in dependency of the module (make Electron Rebuild)
on launch of MM² catch any error when loading the library and display the result (bypass black screen)
sample:

image

But It's not the subject !

I am not a reference, My opinion does not count, I am not an official MagicMirror developer (or 3rd modules developer)

I leave you to make this decision alone
Happy coding!
@bugsounet

@khassel
Copy link
Collaborator

khassel commented Dec 4, 2022

But is the original issue solved (electron rebuild failure) with electron v22?

it is "solved" in the same way as with current v19, you have to use the rebuild of bugsounet.

And will the electron22 requirements be stricter again (fewer architectures supported for instance)?

what do you mean? There are 78 different download options on the release site as for other versions before.

It will be stricter concerning the browser because it contains a newer browser version.

I would like to test it on the develop branch now and if there are problems we can revert back to v19 before next release.

rejas pushed a commit that referenced this issue Dec 8, 2022
Changes:
- as discussed in #2903: update to electron v22 (we can revert it before
next release if we find any problems)
- update other dependencies
- set playwright to version v1.27.1 until #2969 is solved
@rejas
Copy link
Collaborator

rejas commented Dec 10, 2022

what do you mean? There are 78 different download options on the release site as for other versions before.

I just remember vaguely that we were quite conservastive with upgrdaing electron in the past. Was that becuase of the spectron dependency or did that have other reasons too?

@khassel
Copy link
Collaborator

khassel commented Dec 10, 2022

from the change log:

mm 2.20 electron 19
mm 2.18 electron 16
mm 2.17.1 electron hotfix 13.5.1
mm 2.17 electron 13
mm 2.15 electron 11
before electron 3

So I think after upgrading to electron v11 we were not so conservative. We had luck with mm 2.17 because we were using a supported electron version because of the certificate bug (hotfix release 2.17.1).

Thats one reason why I'm not a friend of using an outdated electron version (which gets no updates anymore).

@sdetweil
Copy link
Collaborator Author

in my opinion we don't need to be bleeding edge. we only have dhtml API calls and generally simple web content, one page, normal css. (altho I would like to see has() for css. )

iframes and such.

MichMich added a commit that referenced this issue Jan 1, 2023
## [2.22.0] - 2023-01-01

Thanks to: @angeldeejay, @buxxi, @dariom, @dWoolridge,
@KristjanESPERANTO, @MagMar94, @naveensrinivasan, @retroflex, @SkySails
and @tom.

Special thanks to @khassel, @rejas and @sdetweil for taking over most
(if not all) of the work on this release as project collaborators. This
version would not be there without their effort. Thank you!

### Added

- Added test for remoteFile option in compliments module
- Added hourlyWeather functionality to Weather.gov weather provider
- Removed weatherEndpoint definition from weathergov.js (not used)
- Added css class names "today" and "tomorrow" for default calendar
- Added Collaboration.md
- Added new github action for dependency review (#2862)
- Added a WeatherProvider for Open-Meteo
- Added Yr as a weather provider
- Added config options "ignoreXOriginHeader" and
"ignoreContentSecurityPolicy"

### Removed

- Removed usage of internal fetch function of node until it is more
stable

### Updated

- Cleaned up test directory (#2937) and jest config (#2959)
- Wait for all modules to start before declaring the system ready
(#2487)
- Updated e2e tests (moved `done()` in helper functions) and use es6
syntax in all tests
- Updated da translation
- Rework weather module
- Make sure smhi provider api only gets a maximum of 6 digits
coordinates (#2955)
  - Use fetch instead of XMLHttpRequest in weatherprovider (#2935)
  - Reworked how weatherproviders handle units (#2849)
  - Use unix() method for parsing times, fix suntimes on the way (#2950)
  - Refactor conversion functions into utils class (#2958)
- The `cors`-method in `server.js` now supports sending and recieving
HTTP headers
- Replace `…` by `…`
- Cleanup compliments module
- Updated dependencies including electron to v22 (#2903)

### Fixed

- Correctly show apparent temperature in SMHI weather provider
- Ensure updatenotification module isn't shown when local is _ahead_ of
remote
- Handle node_helper errors during startup (#2944)
- Possibility to change FontAwesome class in calendar, so icons like
`fab fa-facebook-square` works.
- Fix cors problems with newsfeed articles (as far as possible), allow
disabling cors per feed with option `useCorsProxy: false` (#2840)
- Tests not waiting for the application to start and stop before
starting the next test
- Fix electron tests failing sometimes in github workflow
- Fixed gap in clock module when displayed on the left side with
displayType=digital
- Fixed playwright issue by upgrading to v1.29.1 (#2969)

Signed-off-by: naveen <[email protected]>
Co-authored-by: Karsten Hassel <[email protected]>
Co-authored-by: Malte Hallström <[email protected]>
Co-authored-by: Veeck <[email protected]>
Co-authored-by: veeck <[email protected]>
Co-authored-by: dWoolridge <[email protected]>
Co-authored-by: Johan <[email protected]>
Co-authored-by: Dario Mratovich <[email protected]>
Co-authored-by: Dario Mratovich <[email protected]>
Co-authored-by: Magnus <[email protected]>
Co-authored-by: Naveen <[email protected]>
Co-authored-by: buxxi <[email protected]>
Co-authored-by: Thomas Hirschberger <[email protected]>
Co-authored-by: Kristjan ESPERANTO <[email protected]>
Co-authored-by: Andrés Vanegas Jiménez <[email protected]>
@sdetweil
Copy link
Collaborator Author

sdetweil commented Jan 3, 2023

22 does not cause this problem

@sdetweil sdetweil closed this as completed Jan 3, 2023
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

No branches or pull requests

4 participants