Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

deps: add template for generated headers#42616

Closed
danbev wants to merge 6 commits into
nodejs:masternodejs/node:masterfrom
danbev:openssl-3.0.0-header-filesdanbev/node:openssl-3.0.0-header-filesCopy head branch name to clipboard
Closed

deps: add template for generated headers#42616
danbev wants to merge 6 commits into
nodejs:masternodejs/node:masterfrom
danbev:openssl-3.0.0-header-filesdanbev/node:openssl-3.0.0-header-filesCopy head branch name to clipboard

Conversation

@danbev

@danbev danbev commented Apr 5, 2022

Copy link
Copy Markdown
Contributor

OpenSSL 3.0 has a number of header files that are generated, and
currently these headers are copied into the architecture specific
directories. This is done for each asm type, 'asm', 'asm_avx2', and
'no-asm' which has takes up quite a lot of disk space and also becomes
an issue with the headers.tar file which has increased due to this.

This commit adds copies the headers to a common directory for the
architecture, for example with linux-x86_64 there will be a directory
named deps/openssl/config/archs/linux-x86_64/common/include where the headers
will be copied (into subdirectories 'openssl' and 'crypto'. And in the
original locations a header file with the same name will be generated
which points (includes) the common header file.

Fixes: #42081

@nodejs-github-bot nodejs-github-bot added dependencies Pull requests that update a dependency file. needs-ci PRs that need a full CI run. openssl Issues and PRs related to the OpenSSL dependency. labels Apr 5, 2022
@danbev danbev force-pushed the openssl-3.0.0-header-files branch from 47aaa16 to a0b44c6 Compare April 6, 2022 03:35
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@danbev

danbev commented Apr 6, 2022

Copy link
Copy Markdown
Contributor Author

With these changes the size of include/node/openssl/archs is:

$ du -hs node-v18.0.0/include/node/openssl/archs/
37M	node-v18.0.0/include/node/openssl/archs/

I'm not sure if this is acceptable or not but at least it is an improvement current size which is around 61M.

@danbev danbev force-pushed the openssl-3.0.0-header-files branch from a0b44c6 to b9babe6 Compare April 6, 2022 14:27
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@mhdawson

mhdawson commented Apr 6, 2022

Copy link
Copy Markdown
Member

I'm not sure if this is acceptable or not but at least it is an improvement current size which is around 61M.

Definitely a good improvement. I think we should compare to the pre-18.x size to judge whether this is a great first step or the complete fix.

@richardlau

Copy link
Copy Markdown
Member

I'm not sure if this is acceptable or not but at least it is an improvement current size which is around 61M.

Definitely a good improvement. I think we should compare to the pre-18.x size to judge whether this is a great first step or the complete fix.

It's a step in the right direction but still much bigger than when we were using OpenSSL 1.1.1. For comparison the equivalent directories in the headers package for Node.js 16.14.0 is 2.7M (#42081).

@danbev

danbev commented Apr 7, 2022

Copy link
Copy Markdown
Contributor Author

For comparison the equivalent directories in the headers package for Node.js 16.14.0 is 2.7M (#42081).

I've added a comment to #42081 about the reason for the larger size in OpenSSL 3.x, compared to 1.1.1. While there might be more options to cut the size I don't think we will get it down to a size close to that of 1.1.1 as there are now more headers that are generated specifically for an architecture in 3.x than there were for 1.1.1. But I'll take another look and see if there is more that could be done.

@danbev

danbev commented Apr 8, 2022

Copy link
Copy Markdown
Contributor Author

I've been able to find one set of headers that are not being used anymore, and another set for providers which could be shared per architecture like is done in this PR. I'll make these changes and see what the size of the headers.tar is after that.

@mhdawson

mhdawson commented Apr 8, 2022

Copy link
Copy Markdown
Member

@danbev as an FYI some of the failures in the CI do look related to compiling openssl

@danbev

danbev commented Apr 9, 2022

Copy link
Copy Markdown
Contributor Author

@danbev as an FYI some of the failures in the CI do look related to compiling openssl

Thanks, I'll complete the changes I've got and then run through CI again on Monday.

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@danbev

danbev commented Apr 12, 2022

Copy link
Copy Markdown
Contributor Author

With the latest changes the sizes of the sub directories in archs in the produced headers tar file looks like this:

$ ls | xargs du -sch                                                            
1.5M    aix64-gcc-as                                                            
1.5M    aix-gcc                                                                 
1.5M    BSD-x86                                                                 
1.5M    BSD-x86_64                                                              
1.5M    darwin64-arm64-cc                                                       
1.5M    darwin64-x86_64-cc                                                      
1.5M    darwin-i386-cc                                                             
1.5M    linux32-s390x                                                              
1.5M    linux64-mips64                                                             
1.1M    linux64-riscv64                                                            
1.5M    linux64-s390x                                                              
1.5M    linux-aarch64                                                           
1.5M    linux-armv4                                                             
1.5M    linux-elf                                                               
1.5M    linux-ppc                                                               
1.5M    linux-ppc64                                                             
1.5M    linux-ppc64le                                                           
1.5M    linux-x86_64                                                            
1.5M    solaris64-x86_64-gcc                                                    
1.5M    solaris-x86-gcc                                                         
2.8M    VC-WIN32                                                                
2.8M    VC-WIN64A                                                               
936K    VC-WIN64-ARM  
                                                          
35M     total                                                                   

The windows variants are larger but they are also excluded from the headers handling that is part of this PR. We could look into them and see what can be done but if we assume that we can do something similar to what we have done to the others that would probably only get the total size down to something like 34M which I'm not really sure makes that much of a difference.

@richardlau

Copy link
Copy Markdown
Member

With the latest changes the sizes of the sub directories in archs in the produced headers tar file looks like this:

$ ls | xargs du -sch                                                            
1.5M    aix64-gcc-as                                                            
1.5M    aix-gcc                                                                 
1.5M    BSD-x86                                                                 
1.5M    BSD-x86_64                                                              
1.5M    darwin64-arm64-cc                                                       
1.5M    darwin64-x86_64-cc                                                      
1.5M    darwin-i386-cc                                                             
1.5M    linux32-s390x                                                              
1.5M    linux64-mips64                                                             
1.1M    linux64-riscv64                                                            
1.5M    linux64-s390x                                                              
1.5M    linux-aarch64                                                           
1.5M    linux-armv4                                                             
1.5M    linux-elf                                                               
1.5M    linux-ppc                                                               
1.5M    linux-ppc64                                                             
1.5M    linux-ppc64le                                                           
1.5M    linux-x86_64                                                            
1.5M    solaris64-x86_64-gcc                                                    
1.5M    solaris-x86-gcc                                                         
2.8M    VC-WIN32                                                                
2.8M    VC-WIN64A                                                               
936K    VC-WIN64-ARM  
                                                          
35M     total                                                                   

The windows variants are larger but they are also excluded from the headers handling that is part of this PR. We could look into them and see what can be done but if we assume that we can do something similar to what we have done to the others that would probably only get the total size down to something like 34M which I'm not really sure makes that much of a difference.

I'm wondering if we can get rid of some of these. e.g. aix-gcc, darwin-i386-cc, linux32-s390x, linux-ppc, linux-ppc64, solaris-x86-gcc.

@richardlau

Copy link
Copy Markdown
Member

I'm also not sure what linux-armv4 is.

@danbev

danbev commented Apr 12, 2022

Copy link
Copy Markdown
Contributor Author

I'm wondering if we can get rid of some of these. e.g. aix-gcc, darwin-i386-cc, linux32-s390x, linux-ppc, linux-ppc64, solaris-x86-gcc.

I don't have the answer to that question, but I'd be all for removing them if they are no longer needed 👍

@mhdawson

Copy link
Copy Markdown
Member

@miladfarca can you check with Vascili on which of we need?

1.5M aix64-gcc-as
1.5M aix-gcc

@mhdawson

Copy link
Copy Markdown
Member

I think we could get rid of linux-ppc and linux-ppc64 as we only support linux-ppc64le as long as that is what is used for linux-ppc64 builds.

@miladfarca

miladfarca commented Apr 12, 2022

Copy link
Copy Markdown
Contributor

@miladfarca can you check with Vascili on which of we need?

1.5M aix64-gcc-as 1.5M aix-gcc

I've forwarded the link to Vasili to confirm.
Assuming -maix64 is used during build with gcc (like how its done on the V8 side) then 64 bit headers might need to be used i.e aix64-gcc-as.

danbev added a commit that referenced this pull request Apr 28, 2022
PR-URL: #42616
Fixes: #42081
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
danbev added a commit that referenced this pull request Apr 28, 2022
PR-URL: #42616
Fixes: #42081
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
@danbev

danbev commented Apr 28, 2022

Copy link
Copy Markdown
Contributor Author

Landed in 33bc537, 4f1f14e, d8365ac, d68c4e9, 3f0b2a2, and 4ecfe3f

@danbev danbev closed this Apr 28, 2022
targos pushed a commit that referenced this pull request May 2, 2022
OpenSSL 3.0 has a number of header files that are generated, and
currently these headers are copied into the architecture specific
directories. This is done for each asm type, 'asm', 'asm_avx2', and
'no-asm' which has takes up quite a lot of disk space and also becomes
an issue with the headers.tar file which has increased due to this.

This commit adds copies the headers to a common directory for the
architecture, for example with linux-x86_64 there will be a directory
named deps/openssl/config/archs/linux-x86_64/common/include where the
headers will be copied (into subdirectories 'openssl' and 'crypto'.
And in the original locations a header file with the same name will be
generated which points (includes) the common header file.

PR-URL: #42616
Fixes: #42081
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
targos pushed a commit that referenced this pull request May 2, 2022
PR-URL: #42616
Fixes: #42081
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
targos pushed a commit that referenced this pull request May 2, 2022
This arch was renamed to clarify that it used the aix assembler (as) and
not the gnu assembler. It was removed from the Makefile and not being
built but would still be picked up by make targets like the header-tar
target.

PR-URL: #42616
Fixes: #42081
Refs: openssl/openssl@178fa72ed5
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
targos pushed a commit that referenced this pull request May 2, 2022
PR-URL: #42616
Fixes: #42081
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
targos pushed a commit that referenced this pull request May 2, 2022
PR-URL: #42616
Fixes: #42081
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
targos pushed a commit that referenced this pull request May 2, 2022
PR-URL: #42616
Fixes: #42081
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
@targos targos mentioned this pull request May 2, 2022
@yanovich

yanovich commented May 5, 2022

Copy link
Copy Markdown

commit 7fae2c9 breaks C++ addons which use any openssl header:

$ yarn build 
yarn run v1.22.18
$ node-gyp configure --silent && node-gyp build --silent
make: Entering directory '/home/user/src/addon/build'
  CXX(target) Release/obj.target/addon/addon.o
In file included from /home/s/.cache/node-gyp/18.1.0/include/node/openssl/opensslconf.h:9,
                 from /home/s/.cache/node-gyp/18.1.0/include/node/openssl/macros.h:14,
                 from /home/s/.cache/node-gyp/18.1.0/include/node/openssl/evp.h:14,
                 from ../addon.cc:5:
/home/s/.cache/node-gyp/18.1.0/include/node/openssl/./opensslconf_asm.h:97:11: fatal error: ./archs/linux-x86_64/asm/include/openssl/opensslconf.h: No such file or directory
   97 | # include "./archs/linux-x86_64/asm/include/openssl/opensslconf.h"
      |           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
compilation terminated.
make: *** [addon.target.mk:120: Release/obj.target/addon/addon.o] Error 1
$ head -n 5 addon.cc
#include <stdlib.h>
#include <string.h>
#include <iostream>

#include <openssl/evp.h>

@juanarbol

Copy link
Copy Markdown
Member

This is a dep of #42356, so, this can not be landed in v16.x

Renegade334 added a commit to Renegade334/node that referenced this pull request Feb 15, 2026
nodejs-github-bot pushed a commit that referenced this pull request Feb 19, 2026
Refs: #42616
PR-URL: #61834
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
aduh95 pushed a commit that referenced this pull request Feb 22, 2026
Refs: #42616
PR-URL: #61834
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
aduh95 pushed a commit that referenced this pull request Apr 4, 2026
Refs: #42616
PR-URL: #61834
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
aduh95 pushed a commit that referenced this pull request Apr 5, 2026
Refs: #42616
PR-URL: #61834
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
aduh95 pushed a commit that referenced this pull request Apr 5, 2026
Refs: #42616
PR-URL: #61834
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
aduh95 pushed a commit that referenced this pull request Apr 6, 2026
Refs: #42616
PR-URL: #61834
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
aduh95 pushed a commit that referenced this pull request Apr 8, 2026
Refs: #42616
PR-URL: #61834
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

dependencies Pull requests that update a dependency file. needs-ci PRs that need a full CI run. openssl Issues and PRs related to the OpenSSL dependency.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Node.js OpenSSL headers much larger after OpenSSL 3 switch over

9 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.