build: headers tarball target#1975
Uh oh!
There was an error while loading. Please reload this page.
Conversation
rvagg commented Jun 14, 2015
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. |
rvagg commented Jun 14, 2015
also, the .tar.gz is 149K and .tar.xz is 125K |
jbergstroem commented Jun 14, 2015
I like this. If we're to use |
rvagg commented Jun 15, 2015
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. |
jbergstroem commented Jun 15, 2015
The changes to Makefile looks good to me. Anyone else? |
retrohacker commented Jun 15, 2015
lgtm, albeit I don't know enough about Makefiles to say with confidence something silly isn't being done. |
rmg commented Jun 15, 2015
@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 |
rvagg commented Jun 16, 2015
@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 |
rvagg commented Jun 18, 2015
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 |
indutny commented Jun 18, 2015
Gosh, I think we need to make them symlinks and resolve them on copying. @shigeki what do you think? |
shigeki commented Jun 18, 2015
@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. |
indutny commented Jun 18, 2015
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? |
rvagg commented Jun 19, 2015
@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 |
shigeki commented Jun 19, 2015
@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. |
shigeki commented Jun 19, 2015
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
rvagg commented Jun 23, 2015
can I get any more reviewers here on 6ae463c? @nodejs/build? |
brendanashworth commented Jun 24, 2015
rvagg commented Jun 24, 2015
Nope, tangential |
rvagg commented Jun 25, 2015
upstream PR was merged, this PR is now only a single commit and easier to review, can I get some eyes please? |
jbergstroem commented Jun 25, 2015
LGTM assuming we get #2016 in first. |
8 participants
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