-
Notifications
You must be signed in to change notification settings - Fork 109
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
Remove vulnerable packages in net8.0 webassembly image #1291
Conversation
This matches what we do upstream in emsdk in net9.0+
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.
Thank you!
|
||
RUN mkdir ${EMSCRIPTEN_PATH} \ | ||
&& cd ${EMSCRIPTEN_PATH} \ | ||
&& git clone https://github.com/emscripten-core/emsdk.git ${EMSDK_PATH} \ | ||
&& cd ${EMSDK_PATH} \ | ||
&& git checkout ${EMSCRIPTEN_VERSION} \ | ||
# patch node version in emsdk_manifest.json | ||
&& sed -i 's/14\.18\.2/15\.14\.0/g' emsdk_manifest.json \ |
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.
Can this be generalized so it doesn't need to maintained as versions get updated?
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.
you mean in a variable? we can't just pick any node version since it needs to be one that emsdk shipped.
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.
I mean use a regex to match on the existing version (not a hardcoded version but one that has a simple version pattern) defined in the JSON file and replace it with the new value, referencing an ENV
value that could be defined.
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.
I'm not sure why that'd be necessary given that emsdk 3.1.34 will always ship with node 14.18.2 and as mentioned in #1281 we can't upgrade the emsdk version.
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.
or do you mean the replacement version?
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.
I'm referring to both versions so that neither have to be hardcoded on this line.
emsdk 3.1.34 will always ship with node 14.18.2
So when the emsdk version is updated, that requires knowing what the node version is and having to update this line with that value. My aim here is to reduce the amount of work needed to rev the versions.
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.
So when the emsdk version is updated, that requires knowing what the node version is and having to update this line with that value.
No, what I'm saying is the emsdk version is frozen for net8, we can't update it. And this logic is gone in net9+
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.
Ok, got it. Thanks
This matches what we do upstream in emsdk in net9.0+