build: headers tarball target #1975
Conversation
|
I forgot to mention that this builds on #1938 so there's only one commit to review, rvagg@9fd0f64, it only touches Makefile and install.py. |
|
also, the .tar.gz is 149K and .tar.xz is 125K |
|
I like this. If we're to use |
|
I believe that we only need to add include/node to the include_dirs for node-gyp and leave the old ones in place and we'd get backward compatibility. |
|
The changes to Makefile looks good to me. Anyone else? |
|
lgtm, albeit I don't know enough about Makefiles to say with confidence something silly isn't being done. |
@jbergstroem @rvagg this reminds me of the one-line change nodejs/node-gyp#492 that got shot down when I proposed it. My goal was to just use the headers from the binary distribution so that nothing needs to be downloaded to |
|
@rmg yes, similar but this is a step towards that goal, the next part of that is making node-gyp aware of where node is installed and where to look for the headers so it doesn't have to download - but as I've said, it will always need to have the ability to download for situations where it can't find local headers |
|
I've just discovered a disturbing blocker for this: 6b3df92 And this is the same with the header files generated by this change as well because it does the same thing as install.py. i.e. we are shipping with broken openssl headers so any package that needs to compile against them (such as bignum, a very popular package) won't work. @shigeki && @indutny can you explain what these header rewrites are about and suggest a compromise whereby install.py can install the actual openssl headers? Further, it looks like 0.10 and 0.12 aren't even shipping with openssl headers, I have no idea how we're getting them in there and joyent/node still isn't: (compare to |
|
Gosh, I think we need to make them symlinks and resolve them on copying. @shigeki what do you think? |
|
@rvagg Sorry, I missed this mention. I should had been aware of this in reviewing #1354. The current master of openssl-1.1.0 has already move to put real header files not symlinks. Symlinks cause issues on Windows in extracting source files. Probably we have three options,
I think third one is easy and simple. |
|
Huuuh? If we will move real header files there, what's the point in next two steps? Why can't we just copy them to the installation folder? |
|
@indutny they are options not steps. @shigeki my only preference is that whatever we do is easy to maintain and won't get lost during future updates (this will be easy to overlook). A |
|
@rvagg Yes, I totally agree it. I wonder if there is no side effects move all real header files into the include dir. I take a look and test it today. |
|
Looking at openssl/openssl@dee502b , moving real header files breaks Configure and some util scripts. Although they are not used in gyp build but I think copying them into |
On upgrading openssl, all symlinks in pulic header files are replaced with nested include files. The issue was raised that installing them leads to lost its references to real header files. To avoid this, all public header files are copied into the `deps/openssl/openssl/include/openssl/` directory. As a result, we have duplicated header files under `deps/openssl/openssl/` but copied files are refereed in build as specified to include path in openssl.gyp. Fixes: nodejs#1975
|
can I get any more reviewers here on 6ae463c? @nodejs/build? |
|
Nope, tangential |
|
upstream PR was merged, this PR is now only a single commit and easier to review, can I get some eyes please? |
|
LGTM assuming we get #2016 in first. |
For
make tar-headers(plus the other options in #1938 for nightlies etc.). Makes a tarball with the headers as they are packaged in binary tarballs - which is different to the source tarball but is better for being used as an includes path for node-gyp et. al.We will need to get node-gyp to download this file from release directories where it's available. The way I suggest we do that is via #493 which is my next task (YES, YES IT IS!); if node-gyp detects a
process.release.headers_url(or whatever it ends up being) it knows to download from there and unpack to ~/.node-gyp/. Then we'd need to prependinclude/nodeto the list of include directories in addon.gyp (https://github.com/TooTallNate/node-gyp/blob/56b7f9cff21b74421830cf2324545c8420f28e53/addon.gypi#L7-L11) so that it picks up this new directory format but still allows for the old format.It's worth noting for anyone not aware of this fact but these includes are included in the binary downloads and have been from early 0.10 for all but Windows, so node-gyp shouldn't need to even download anything most of the time. Making node-gyp aware of platform headers is a separate task and should be done but we will always need to make headers available for download in case you're using an executable that hasn't been installed with headers for whatever reason.
/R=@nodejs/build @TooTallNate