Skip to content
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 unused openssl headers #1923

Merged
merged 2 commits into from
Nov 13, 2023

Conversation

yehonatanz
Copy link
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.

Dockerfile-slim.template Outdated Show resolved Hide resolved
@nschonni
Copy link
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
Copy link
Member

LaurentGoderre commented Sep 14, 2023

@yehonatanz could you rebase and fix the conflict?

@yehonatanz
Copy link
Contributor Author

@yehonatanz could you rebase and fix the conflict?

Done

@yehonatanz
Copy link
Contributor Author

@LaurentGoderre rebased again now

Copy link
Contributor

@yosifkit yosifkit left a comment

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
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
Contributor Author

@tianon Any action item for me?

@LaurentGoderre LaurentGoderre merged commit dbc1745 into nodejs:main Nov 13, 2023
8 checks passed
Copy link

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
Member

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

@yehonatanz
Copy link
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
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.

None yet

6 participants