-
Notifications
You must be signed in to change notification settings - Fork 398
Patch Javascript Build Packages #2557
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
base: main
Are you sure you want to change the base?
Conversation
@ashwinbhat , @jstone-lucasfilm : I'm not sure if it's too late for this change, but I think it would be better to patch this to have a known safe version of the packages used for the JS build. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lock these to avoid version "creep". New versions can make it so that tests won't run.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix this as version "creep" has already occurred. This is 100% compatible with the NodeJS version we are locked on for builds (16.20.2). I set this to test locally on Windows.
@@ -5,8 +5,8 @@ | |||
@rem Edit the following paths to match your local locations for the Emscripten and MaterialX projects. | |||
set EMSDK_LOCATION=C:/GitHub/emsdk | |||
set MATERIALX_LOCATION=C:/GitHub/MaterialX | |||
call %EMSDK_LOCATION%/emsdk.bat install latest | |||
call %EMSDK_LOCATION%/emsdk.bat activate latest | |||
call %EMSDK_LOCATION%/emsdk.bat install 2.0.20 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should never have been used as newer versions can be incompatible and not build the C++ code due to API changes.
This change sounds very reasonable to me, @kwokcb, though since it's a significant number of edits, I'd feel more confident waiting until 1.39.5 to move forward with this. |
Updates
Fix the packages used for MaterialXTest and MaterialXView so that they are compatible with the current version of NodeJS used (16.20.2).
The default emscripten 2.0.20 utility script sets NodeJS version to a version which is too old and will result in possible incompatibilities.
Lock the packages versions until such time as NodeJS version is changed.
Note This does not try to bump any version of emcripten or NodeJS, it just fixes so that the appropriate packages are now used.
Fixes: Web Viewer : Packages are incompatible using Node version 16. #2547, WebPack Packages are incompatible (won't run) for MaterialXView #2545
Tests
Documentation