search

emscripten-core / emsdk

Emscripten SDK
http://emscripten.org
Other
3.02k stars 688 forks source link

Update node, 15.14.0 -> 16.20.0 #1232

Closed dschuff closed 1 year ago

dschuff commented 1 year ago

This allows us to use the native aarch64 version on MacOS

dschuff commented 1 year ago

This seems to be working other than the fact that the tests use some versions that aren't supported on arm64 (e.g. 1.39.15 in test.sh). How important is it to test such an old version? Should we keep the very old versions (and just skip that test on M1) or update to a more recent version?

sbc100 commented 1 year ago

This seems to be working other than the fact that the tests use some versions that aren't supported on arm64 (e.g. 1.39.15 in test.sh). How important is it to test such an old version? Should we keep the very old versions (and just skip that test on M1) or update to a more recent version?

I think just skip that part of the test when running on arm64

sbc100 commented 1 year ago

Hopefully this is a step towards https://github.com/emscripten-core/emsdk/issues/1173

sbc100 commented 1 year ago

I wonder if we should announce this change somewhere? I guess its mostly internal facing, but some folks might be using the emsdk version of node directly I guess?

dschuff commented 1 year ago

Yeah, it's probably worth mentioning. Maybe someone would want to disable Rosetta or something and we are the only thing holding them back. In any case it seems worth announcing the node update, and the architecture update would go with that.

sbc100 commented 1 year ago

Yeah, it's probably worth mentioning. Maybe someone would want to disable Rosetta or something and we are the only thing holding them back. In any case it seems worth announcing the node update, and the architecture update would go with that.

I'm just not sure we have a good place to announce such changes.

The other thing that I hadn't previously considered is that this change retro-actively upgrade node for all old versions of emsdk.. so there is a risk we could break old things that used to work. We should be a little careful here.. its a shame we don't have a good mechanism for using this new version only with newer versions of the sdk.

dschuff commented 1 year ago

Hm, yeah. I guess this could only happen if someone uses a new version of emsdk to download an old version of emscripten, and it in theory should only affect the code that emscripten itself runs, right? Hopefully not too many users are using emsdk's node for running their own tests or tooling or something?

dschuff commented 1 year ago

I just realized that test.py runs under python2 on Windows, and we flake8 on both py2 and py3. Now that emscripten requires python3, can we drop py2? (I would make that separate from this change, but just wondering if that was intentional).

dschuff commented 1 year ago

OK, all the tests are passing now! given the above comment, do you still think this is a good idea?

sbc100 commented 1 year ago

Which comment are you talking about?

I do think this is a good idea yes. If it proves problematic to use node 16 with older versions of emsdk we can figure out a way to install node 15 for older versions.. but in the past when we have done these upgrades it hasn't been a problem (IIRC).

sbc100 commented 1 year ago

I think we can land this now.

juj commented 1 year ago

Thanks for the ping. Looks good, and we should be good on this front overall as well.

Btw, I will be on vacation until early August, so my response time will be delayed up a bit until then.