-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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
test: add openssl add-on test #6274
Conversation
Sadly this failed on windows. :-/ => https://ci.nodejs.org/job/node-test-binary-windows/1795/RUN_SUBSET=1,VS_VERSION=vcbt2015,label=win10/ |
It's the age-old problem of the windows build not exporting openssl symbols. I'm going to tinker with |
dc38884
to
5d04868
Compare
I haven't had much luck on Windows so far. I added a
@nodejs/platform-windows Ideas? @nodejs/build Is there a way to extract build artifacts from the buildbots? The node.lib and node.exp files in particular? |
5d04868
to
f56b9be
Compare
7da4fd4
to
c7066fb
Compare
541005c
to
019e97b
Compare
@bnoordhuis what do you need here exactly? You want the build artifacts after the jobs are finished or beforehand? Are you wanting to inspect artifacts to debug the job? If that's what you're after we have to instruct Jenkins what files to grab using globs |
After. I'd like the node.lib and the node.exp files that are built. The binary itself might come in handy, too. |
@bnoordhuis do you want the files to download so you can check them locally or do you want them to be present in the test machines? There is no automated way to get the files from the CI slaves. If you want the files let me know exactly what to build (this PR head?) and I can email them to you. If you want the files on the test machines, you can try https://ci.nodejs.org/job/node-test-commit-windows/ , that builds and tests in the same job. If you can make that work, I'll make it work with the fanned job. |
Thanks, I was hoping I could download the files somewhere, but I'll tweak this PR some more and ping you when I need them. |
I may have inadvertently broken the Windows CI with my latest update, all the bots are hanging at the "Building add-ons" step now... https://ci.nodejs.org/job/node-test-binary-windows/2077/ |
@bnoordhuis when that happens, please abort the job, it's no big issue. I tried to build this locally, and |
You mean it's not an executable but a library? Le sigh, it must be something that gyp is doing. I'm going to try it one more time in case the CI was in a funk yesterday: https://ci.nodejs.org/job/node-test-pull-request/2607/ |
Any news? |
I took a peek at this and it seems to be two things: OpenSSL and zlib. Both of these are missing from the symbol table (dumpbin /EXPORTS node.exe). I ran the vcbuild.bat which generated the VS solution and then I just looked at the project and couldn't really see any reference to any .def file. So I just went to Project -> Properties -> Linker -> Input -> Module Definition File and put the path to the zlib.def file as a test and built the program. Now I get all the zlib functions exported and I can see there are inflate and such functions in node.lib. So maybe you could just focus on generating one .def file with both openssl and zlib functions and just pass that one file as /DEF:filename? V8 and uv are already exported but I couldn't see any .def file so maybe you are using __declspec(dllexport) or something - I don't know but passing the zlib.def worked and I can send you my node.lib and node.exe if you want to compare the symbol table. |
What is the status on this? If things get way to complex, wouldn't it be pretty simple to manually run the mkdef perl script offline and commit as one fix .def file? Or just write a new script that just scrapes functions from the documentation. With |
I just built a node.exe that exports all OpenSSL symbols and runs as an executable. The size is about 700kb larger than normal and RAND_poll is listed when dumpbin / exports. I did not use /FORCE:UNDEFINED, only /DEF. I ended up writing a "script" that exports the symbols from openssl.lib (I couldn't scrape the symbols from the documentation because some of the symbols were just macros and thus not real symbols). I could commit my node.def file and add instructions on how to generate it (including code). Basically I used |
My plan was to restrict what we export to just public symbols. Right now we export everything on UNIX platforms. |
You could do a combination where you scrape the documentation + limit the symbols to what is in openssl.lib. That way macros are not added and only the documented public symbols are added. I have a tool that can scrape all the public symbols and also limit them to what is in openssl.lib. To get the /DEF option added to the VC++ node project I did this in node.gyp:
As an addition after the 'VCManifestTool' under 'msvs_settings'. It shows up in Visual Studio and I'm building it right now. |
Unrelated flake, fortunately. Barring nays from other reviewers I'll merge this tomorrow. |
Export symbols from the bundled openssl for add-ons to link against. Fixes: nodejs/node-v0.x-archive#4051 PR-URL: nodejs#6274 Reviewed-By: James M Snell <[email protected]>
Export symbols from the bundled openssl for add-ons to link against. Fixes: nodejs/node-v0.x-archive#4051 PR-URL: #6274 Reviewed-By: James M Snell <[email protected]>
@bnoordhuis There's a bug in the mkssldef.py script. I'm missing SSL_set_fd and two others. They are present in the ssleay.num file but have a different format of the rightmost column. Most symbols are present now in 6.3.0 though. |
This is my output when compiling on Windows with 6.3.0:
|
@bnoordhuis this does not land cleanly. It will require a backport. |
#7676 - TBD if it should land. |
I don't see an actual commit link here so I dug it up: b4d4fd9 |
Export symbols from the bundled openssl for add-ons to link against. Fixes: nodejs/node-v0.x-archive#4051 PR-URL: nodejs#6274 Reviewed-By: James M Snell <[email protected]>
Export symbols from the bundled openssl for add-ons to link against. Fixes: nodejs/node-v0.x-archive#4051 PR-URL: #6274 Reviewed-By: James M Snell <[email protected]>
Add a smoke test for checking that linking to the bundled openssl from
an add-on works.
CI 1st attempt: https://ci.nodejs.org/job/node-test-pull-request/2315/
CI 2nd attempt: https://ci.nodejs.org/job/node-test-pull-request/2316/
CI 3rd attempt: https://ci.nodejs.org/job/node-test-pull-request/2373/
CI 4th attempt: https://ci.nodejs.org/job/node-test-pull-request/2378/
CI 5th attempt: https://ci.nodejs.org/job/node-test-pull-request/2407/
CI 6th attempt: https://ci.nodejs.org/job/node-test-pull-request/2408/
CI 7th attempt: https://ci.nodejs.org/job/node-test-pull-request/2581/
CI 8th attempt: https://ci.nodejs.org/job/node-test-pull-request/2607/
CI 9th attempt: https://ci.nodejs.org/job/node-test-pull-request/3168/
CI 10th attempt: https://ci.nodejs.org/job/node-test-pull-request/3169/
CI 11th attempt: https://ci.nodejs.org/job/node-test-pull-request/3170/