Issue building for arm/v7 in GitHub Actions

[jrasm91]

Ever since the postinstall script was addeed, we've been unable to build for arm/v7, which is a 32-bit operating architecture.

https://github.com/immich-app/immich/actions/runs/4526005132/jobs/7970879554?pr=2088

#22 913.8 npm ERR! path /usr/src/app/node_modules/local-reverse-geocoder
#22 913.8 npm ERR! command failed
#22 913.8 npm ERR! signal SIGTRAP
#22 913.8 npm ERR! command sh -c node --max-old-space-size=4096 postinstall.js
#22 913.8 npm ERR! <--- Last few GCs --->
#22 913.8 npm ERR! 
#22 913.8 npm ERR! 
#22 913.9 npm ERR! <--- JS stacktrace --->
#22 913.9 npm ERR! 
#22 913.9 npm ERR! 
#22 913.9 npm ERR! #
#22 913.9 npm ERR! # Fatal javascript OOM in GC during deserialization
#22 913.9 npm ERR! #
#22 913.9 
#22 913.9 npm ERR! A complete log of this run can be found in:
#22 913.9 npm ERR!     /root/.npm/_logs/2023-03-26T18_26_11_167Z-debug-0.log
#22 ERROR: process "/bin/sh -c npm ci" did not complete successfully: exit code: 1

We don't have any of the environment variables set for, or make use of, the postinstall script in anyway, but it is setting available memory to 4GiB, which I believe for some reason is leading to this OOM error.

Do you have any suggestions as to how we can install and use this library for ARMv7 without disabling all npm scripts? Is there a specific reason why you chose to use a postinstall script to add this feature, instead of letting the calling repository manually call postinstall if necessary? And, if so, why it is hardcoded to 4GiB instead of the node lts default value?

[tomayac]

The postinstall script was introduced in the context of https://github.com/tomayac/local-reverse-geocoder/pull/60. I guess the 4GB memory setting was determined experimentally?! @itaisatati may remember. I'm happy to merge PRs that improve upon the status quo.

[jrasm91]

It looks like it's a specific issue with running node on a 32 bit architecture. Testing in a VM we were able to reproduce an OOM error when using --max-old-space-size=4096, while the issue went away with --max-old-space-size=4095.

[tomayac]

Looked this up in the docs. Nothing specific to be found. But 32 bit processes can only address up to a max of 4 gigs of memory address space, so this looks like it's to be expected.

[tomayac]

Maybe the postinstall script can determine the bitness of the OS and then set the option accordingly? Do you want to investigate what needs to be done?

[jrasm91]

It's a tricky problem. Personally, I don't believe a library should be controlling the memory requirements of the runtime in a postinstall script. Just sounds like a recipe for disaster. It seems like you could accomplish the same thing by just manually calling node node_modules/local-reverse-geocode/postinstall.js from your own code/postinstall hook and removing that line from local-reverse-geocoders package.json.

I would suggest either:

1. Removing the hook and updating the documentation on the appropriate use of this feature (as a breaking change) or
2. Leave it as is, since it seems to work fine, but change the size to 4095 so it works as expected on 32 bit arches and don't change anything else.

[tomayac]

1. looks like the cleaner alternative to me. @itaisatati, would this work for you, too?

[itaisatati]

Hi @tomayac @jrasm91 I used the 4GB value because some configurations caused the process to start swapping (only tested on 64bit systems) Both options would work for me @aminmahrami would it also work for you? (he requested the feature initially)

[itaisatati]

LMK if you need my assistance with the change

[tomayac]

@itaisatati, sorry for the delay, I was OoO.

LMK if you need my assistance with the change

If you want to open a PR that changes it in the desired direction, that'd be appreciated!

[tomayac]

Fixed via #69 for now. We can re-open this should folks depend on a non-dirty fix.

[tomayac]

Released v0.15.1 that includes this fix. Thank you!