Sitelet https://github.com/nodejs/docker-node/pull/1923
Skip to content

Remove unused openssl headers - #1923

Merged
LaurentGoderre merged 2 commits into
nodejs:mainfrom
yehonatanz:remove-unused-openssl-headers
Nov 13, 2023
Merged

LaurentGoderre merged 2 commits into
nodejs:mainfrom
yehonatanz:remove-unused-openssl-headers

Conversation

@yehonatanz

Copy link
Copy Markdown
Contributor

Remove unused OpenSSL headers for architectures other than current one

Description

Remove unused OpenSSL headers for platforms other than current one (linux + machine_arch).
This is a mitigation for this open NodeJS issue.

Motivation and Context

To save a few 10s of MB in all slim images:

image

Testing Details

I tested the following command ran successfully:

docker run node:18-buster-slim-in-this-branch node -e "console.log(require('crypto').createHash('sha256').update('bla').digest('hex'))"

Types of changes

  • Documentation
  • Version change (Update, remove or add more Node.js versions)
  • Variant change (Update, remove or add more variants, or versions of variants)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Other (none of the above)

Checklist

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING.md document.
  • All new and existing tests passed.

Comment thread Dockerfile-slim.template Outdated
@nschonni

Copy link
Copy Markdown
Member

Some previous discussions on this #1776

@yehonatanz
yehonatanz force-pushed the remove-unused-openssl-headers branch from c880f5a to f71a299 Compare June 28, 2023 12:38
@yehonatanz
yehonatanz force-pushed the remove-unused-openssl-headers branch from f71a299 to f863281 Compare August 6, 2023 15:31
@LaurentGoderre

LaurentGoderre commented Sep 14, 2023 •

Copy link
Copy Markdown
Member

@yehonatanz could you rebase and fix the conflict?

@yehonatanz
yehonatanz force-pushed the remove-unused-openssl-headers branch from f863281 to 2c45783 Compare September 14, 2023 16:17
@yehonatanz

Copy link
Copy Markdown
Contributor Author

@yehonatanz could you rebase and fix the conflict?

Done

@yehonatanz
yehonatanz force-pushed the remove-unused-openssl-headers branch from 2c45783 to 924180b Compare September 27, 2023 13:07
@yehonatanz

Copy link
Copy Markdown
Contributor Author

@LaurentGoderre rebased again now

@yehonatanz
yehonatanz requested a review from tianon October 2, 2023 12:57

@yosifkit yosifkit left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@nodejs/docker, it might be good to add the same change to alpine and the non-slim images.

@LaurentGoderre

Copy link
Copy Markdown
Member

A similar change in node itself had broken gyp dependencies when they used openssl headers. I tried to reproduce using node-bcrypt and it was building fine.

@yehonatanz

Copy link
Copy Markdown
Contributor Author

@tianon Any action item for me?

@github-actions

Copy link
Copy Markdown

Created PR on the official-images repo (docker-library/official-images#15700). See https://github.com/docker-library/faq#an-images-source-changed-in-git-now-what if you are wondering when it will be available on the Docker Hub.

@LaurentGoderre

Copy link
Copy Markdown
Member

@yehonatanz would you be able to look into doing the same for the slim and alpine variants if needed?

@yehonatanz

Copy link
Copy Markdown
Contributor Author

@yehonatanz would you be able to look into doing the same for the slim and alpine variants if needed?

Sure, I can take a look at the alpine variants (this PR already covers the slim images)

@yehonatanz

Copy link
Copy Markdown
Contributor Author

@LaurentGoderre Done: #1996

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

Successfully merging this pull request may close these issues.

6 participants