build: fix openssl link error on windows by danbev · Pull Request #13078 · nodejs/node

danbev · 2017-05-17T11:24:09Z

This commit attempts to fix an issue when building on windows using the
following command line options:

.\vcbuild.bat dll debug x64 vc2015

This will result in the following options passed to configure:

configure --debug --shared --dest-cpu=x64 --tag=

This commit excludes the dependency to openssl if node is configured
with --shared.

Also, FP_API to the categories to export in mkssldef when generating
the module definition (openssl.def) allowing the build to compile and
link successfully.

Fixes: #12952

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

build

danbev · 2017-05-17T11:24:50Z

CI: https://ci.nodejs.org/job/node-test-pull-request/8115/

refack · 2017-05-17T12:06:47Z

@danbev did you try to go the complete other way around? i.e.

'dependencies=': [
'<(node_core_target_name)',
],

Just out of intuition...

danbev · 2017-05-17T12:09:36Z

did you try to go the complete other way around? i.e.

There is already a dependency to the node core target, the suggestion here is that it be excluded in this particular case.

refack · 2017-05-17T12:17:18Z

There is already a dependency to the node core target, the suggestion here is that it be excluded in this particular case.

I mean use onlynode.dll as a dependency (that's the = gyp list operator)

danbev · 2017-05-17T12:23:23Z

I mean use only node.dll as a dependency (that's the = gyp list operator)

Ah sorry I understand what you mean now.
I'll give that a try and use the gyp assignment operator (=) to replace the dependency.

danbev · 2017-05-22T13:15:41Z

CI: https://ci.nodejs.org/job/node-test-pull-request/8227/

refack · 2017-05-22T18:47:26Z

I'll test the dll target.

refack · 2017-05-22T19:41:23Z

[question] is this still neccecery?

@refack Yeah I had to make that change. If this is not there that symbol will not be exported (will not end up in <PRODUCT_DIR)\obj\global_intermediate\openssl.def) and a link error will occur when linking cctest.exe. At least that is what I am seeing when building and running this. Where you able to build without this change?

Doesn't adding FP_API to mkssldef_flags in node.gyp work?

I was not aware of that. Let me give that a try. Thanks

That work worked like a charm. Thanks @bnoordhuis
I'll update the PR with the changes later today.

refack

Confirmed dll target builds with vcbuild.bat dll debug x64 vc2015
Rubber stamp libeay.num

refack · 2017-05-23T03:40:36Z

Where you able to build without this change?

I'll check.
But if it's necessary it'll need @shigeki's review.

refack · 2017-05-23T03:45:38Z

P.S. split the commits, so it could be easily floated onto deps/openssl

danbev · 2017-05-23T03:50:29Z

P.S. split the commits, so it could be easily floated onto deps/openssl

No problems, I'll do a rebase later and also remove the first or second commit. I'll leave this as is for now so others can see the options we have to move forward and then clean up the commits. Thanks

refack · 2017-05-23T12:19:17Z

Confirm the need for changing BIO_new_file:

node_crypto.obj : error LNK2019: unresolved external symbol BIO_new_file referenced in function "unsigned long __cdecl node::crypto::AddCertsFromFile(struct x509_store_st *,char const *)" (?AddCertsFromFile@crypto@node@@YAKPEAUx509_store_st@@PEBD@Z) [P:\code\node\cctest.vcxproj]
P:\code\node\Debug\cctest.exe : fatal error LNK1120: 1 unresolved externals [P:\code\node\cctest.vcxproj]

~~In this case I'm +1 on the first method (include [all] exclude [dll])~~

gireeshpunathil

Why renaming the conflicting symbol was never thought as a solution? Circumventing the conflict through build script is not comprehensive in my opinion. We seem to be leveraging a chance that cctest does not require symbols from node core dll - this scenario can change in future and this circuvention will break.

refack · 2017-05-23T13:18:18Z

Why renaming the conflicting symbol was never thought as a solution?

@gireeshpunathil it's not just a one, it's the whole openSSL that gets included twice: #12952 (comment)

gireeshpunathil · 2017-05-23T14:37:21Z

@refack - ah, I see! thanks for the clarification.

refack · 2017-05-23T15:50:55Z

Just checked, these 5 lines are unnecessary to this approach (only dll & gtest).

refack · 2017-05-23T15:52:58Z

+1 for second approach (with the gyp:mkssldef_flags and 5 lines remove)

This commit attempts to fix an issue when building on windows using the following command line options: .\vcbuild.bat dll debug x64 vc2015 This will result in the following options passed to configure: configure --debug --shared --dest-cpu=x64 --tag= This commit excludes the dependency to openssl if node is configured with --shared. Also, FP_API to the categories to export in mkssldef when generating the module definition (openssl.def) allowing the build to compile and link successfully. Fixes: nodejs#12952

danbev · 2017-05-24T05:41:02Z

Rebased and squashed CI: https://ci.nodejs.org/job/node-test-pull-request/8270/

danbev · 2017-05-24T06:59:53Z

test/linux has completed

console output:

TAP Reports Processing: FINISHChecking ^not okNotifying upstream projects of job completionFinished: SUCCESS

refack

Wow, that's minimal!

danbev · 2017-05-25T07:18:15Z

test/linux failure does not look related

console output:

EnvInject] - Loading node environment variables.Building remotely on test-digitalocean-fedora23-x64-1 (fedora23) in workspace /home/iojs/build/workspace/node-test-commit-linux/nodes/fedora23java.lang.NoClassDefFoundError: Could not initialize class com.sun.proxy.$Proxy11	at sun.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)	at sun.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:62)	at sun.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)	at java.lang.reflect.Constructor.newInstance(Constructor.java:423)	at java.lang.reflect.Proxy.newProxyInstance(Proxy.java:739)	at hudson.remoting.RemoteInvocationHandler.wrap(RemoteInvocationHandler.java:143)	at hudson.remoting.Channel.export(Channel.java:618)	at hudson.remoting.Channel.export(Channel.java:587)	at org.jenkinsci.plugins.gitclient.LegacyCompatibleGitAPIImpl.writeReplace(LegacyCompatibleGitAPIImpl.java:198)	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)	at java.lang.reflect.Method.invoke(Method.java:498)	at java.io.ObjectStreamClass.invokeWriteReplace(ObjectStreamClass.java:1118)	at java.io.ObjectOutputStream.writeObject0(ObjectOutputStream.java:1136)	at java.io.ObjectOutputStream.writeObject(ObjectOutputStream.java:348)	at hudson.remoting.UserRequest._serialize(UserRequest.java:158)	at hudson.remoting.UserRequest.serialize(UserRequest.java:167)	at hudson.remoting.UserRequest.perform(UserRequest.java:129)	at hudson.remoting.UserRequest.perform(UserRequest.java:49)	at hudson.remoting.Request$2.run(Request.java:326)	at hudson.remoting.InterceptingExecutorService$1.call(InterceptingExecutorService.java:68)	at java.util.concurrent.FutureTask.run(FutureTask.java:266)	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142)	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617)	at hudson.remoting.Engine$1$1.run(Engine.java:69)	at java.lang.Thread.run(Thread.java:745)	at ......remote call to Channel to /162.243.63.132(Native Method)	at hudson.remoting.Channel.attachCallSiteStackTrace(Channel.java:1545)	at hudson.remoting.UserResponse.retrieve(UserRequest.java:253)	at hudson.remoting.Channel.call(Channel.java:830)Caused: java.io.IOException: Remote call on Channel to /162.243.63.132 failed	at hudson.remoting.Channel.call(Channel.java:838)	at hudson.FilePath.act(FilePath.java:985)Caused: java.io.IOException: remote file operation failed: /home/iojs/build/workspace/node-test-commit-linux/nodes/fedora23 at hudson.remoting.Channel@16fc6ec9:Channel to /162.243.63.132	at hudson.FilePath.act(FilePath.java:992)	at hudson.FilePath.act(FilePath.java:974)	at org.jenkinsci.plugins.gitclient.Git.getClient(Git.java:137)	at hudson.plugins.git.GitSCM.createClient(GitSCM.java:751)	at hudson.plugins.git.GitSCM.createClient(GitSCM.java:743)	at hudson.plugins.git.GitSCM.checkout(GitSCM.java:1101)	at hudson.scm.SCM.checkout(SCM.java:496)	at hudson.model.AbstractProject.checkout(AbstractProject.java:1281)	at hudson.model.AbstractBuild$AbstractBuildExecution.defaultCheckout(AbstractBuild.java:604)	at jenkins.scm.SCMCheckoutStrategy.checkout(SCMCheckoutStrategy.java:86)	at hudson.model.AbstractBuild$AbstractBuildExecution.run(AbstractBuild.java:529)	at hudson.model.Run.execute(Run.java:1728)	at hudson.matrix.MatrixRun.run(MatrixRun.java:146)	at hudson.model.ResourceController.execute(ResourceController.java:98)	at hudson.model.Executor.run(Executor.java:405)Run condition [Always] enabling perform for step [[Execute a set of scripts]]Run condition [Always] enabling perform for step [[Execute a set of scripts]]TAP Reports Processing: STARTLooking for TAP results report in workspace using pattern: *.tapDid not find any matching files. Setting build result to FAILURE.Checking ^not okJenkins Text Finder: File set '*.tap' is emptyNotifying upstream projects of job completionFinished: FAILURE

bnoordhuis · 2017-05-25T08:16:18Z

That's a Jenkins error. The other linux buildbots are green. I think this should be good to go (the more so because the mkssldef target isn't used on UNIX.)

danbev · 2017-05-25T10:25:17Z

I'll merge later this evening or tomorrow morning (public holiday here today) tors 25 maj 2017 kl. 10:16 skrev Ben Noordhuis <notifications@github.com>:

…

That's a Jenkins error. The other linux buildbots are green. I think this should be good to go (the more so because the mkssldef target isn't used on UNIX.) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#13078 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAaY3-uAdSSjspHbRnz2mVXkelpp6507ks5r9TjWgaJpZM4NduZz> .

This commit attempts to fix an issue when building on windows using the following command line options: .\vcbuild.bat dll debug x64 vc2015 This will result in the following options passed to configure: configure --debug --shared --dest-cpu=x64 --tag= This commit excludes the dependency to openssl if node is configured with --shared. Also, FP_API to the categories to export in mkssldef when generating the module definition (openssl.def) allowing the build to compile and link successfully. Fixes: nodejs#12952 PR-URL: nodejs#13078 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Refael Ackermann <refack@gmail.com>

danbev · 2017-05-25T16:42:14Z

Landed in 1cde375

This commit attempts to fix an issue when building on windows using the following command line options: .\vcbuild.bat dll debug x64 vc2015 This will result in the following options passed to configure: configure --debug --shared --dest-cpu=x64 --tag= This commit excludes the dependency to openssl if node is configured with --shared. Also, FP_API to the categories to export in mkssldef when generating the module definition (openssl.def) allowing the build to compile and link successfully. Fixes: #12952 PR-URL: #13078 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Refael Ackermann <refack@gmail.com>

This commit attempts to fix an issue when building on windows using the following command line options: .\vcbuild.bat dll debug x64 vc2015 This will result in the following options passed to configure: configure --debug --shared --dest-cpu=x64 --tag= This commit excludes the dependency to openssl if node is configured with --shared. Also, FP_API to the categories to export in mkssldef when generating the module definition (openssl.def) allowing the build to compile and link successfully. Fixes: nodejs#12952 PR-URL: nodejs#13078 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Refael Ackermann <refack@gmail.com>

This commit attempts to fix an issue when building on windows using the following command line options: .\vcbuild.bat dll debug x64 vc2015 This will result in the following options passed to configure: configure --debug --shared --dest-cpu=x64 --tag= This commit excludes the dependency to openssl if node is configured with --shared. Also, FP_API to the categories to export in mkssldef when generating the module definition (openssl.def) allowing the build to compile and link successfully. Fixes: nodejs#12952 PR-URL: nodejs#13078 Backport-PR-URL: nodejs#12948 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Refael Ackermann <refack@gmail.com>

This commit attempts to fix an issue when building on windows using the following command line options: .\vcbuild.bat dll debug x64 vc2015 This will result in the following options passed to configure: configure --debug --shared --dest-cpu=x64 --tag= This commit excludes the dependency to openssl if node is configured with --shared. Also, FP_API to the categories to export in mkssldef when generating the module definition (openssl.def) allowing the build to compile and link successfully. Fixes: #12952 PR-URL: #13078 Backport-PR-URL: #12948 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Refael Ackermann <refack@gmail.com>

nodejs-github-bot added the build Issues and PRs related to build files or the CI. label May 17, 2017

mscdex added the windows Issues and PRs related to the Windows platform. label May 17, 2017

refack self-requested a review May 22, 2017 19:40

refack reviewed May 22, 2017
View reviewed changes

refack approved these changes May 22, 2017
View reviewed changes

gireeshpunathil requested changes May 23, 2017
View reviewed changes

gireeshpunathil approved these changes May 23, 2017
View reviewed changes

refack reviewed May 23, 2017
View reviewed changes

danbev force-pushed the build-win-cctest-link-error branch from 5f86152 to 13515e5Compare May 24, 2017 05:37

danbev changed the title ~~build: don't link node.dll for cctest target~~build: fix openssl link error on windowsMay 24, 2017

bnoordhuis approved these changes May 24, 2017
View reviewed changes

refack approved these changes May 24, 2017
View reviewed changes

danbev closed this May 25, 2017

danbev deleted the build-win-cctest-link-error branch May 25, 2017 16:43

jasnell mentioned this pull request May 28, 2017
8.0.0 Release Proposal #12220
Closed

gibfahn mentioned this pull request Jun 15, 2017
Auditing for 6.11.1 nodejs/Release#230
Closed
3 tasks

refack mentioned this pull request Jul 13, 2017
doc: better document --shared build #14158
Closed

MylesBorins mentioned this pull request Jul 18, 2017
v6.11.2 proposal #14356
Merged

refack added the embedding Issues and PRs related to embedding Node.js in another project. label Aug 17, 2017

refack mentioned this pull request Sep 6, 2017
Linker error, while compiling node js from source (as a shared library) - v 6.11.2 #15195
Closed

Uh oh!

Uh oh!

Conversation

danbev commented May 17, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

danbev commented May 17, 2017

Uh oh!

refack commented May 17, 2017

Uh oh!

danbev commented May 17, 2017

Uh oh!

refack commented May 17, 2017

Uh oh!

danbev commented May 17, 2017

Uh oh!

danbev commented May 22, 2017

Uh oh!

refack commented May 22, 2017

Uh oh!

refackMay 22, 2017

Choose a reason for hiding this comment

Uh oh!

danbevMay 23, 2017

Choose a reason for hiding this comment

Uh oh!

bnoordhuisMay 23, 2017

Choose a reason for hiding this comment

Uh oh!

danbevMay 23, 2017

Choose a reason for hiding this comment

Uh oh!

danbevMay 23, 2017

Choose a reason for hiding this comment

Uh oh!

refack left a comment

Choose a reason for hiding this comment

Uh oh!

refack commented May 23, 2017

Uh oh!

refack commented May 23, 2017

Uh oh!

danbev commented May 23, 2017

Uh oh!

refack commented May 23, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gireeshpunathil left a comment

Choose a reason for hiding this comment

Uh oh!

refack commented May 23, 2017

Uh oh!

gireeshpunathil commented May 23, 2017

Uh oh!

refackMay 23, 2017

Choose a reason for hiding this comment

Uh oh!

danbevMay 24, 2017

Choose a reason for hiding this comment

Uh oh!

refack commented May 23, 2017

Uh oh!

danbev commented May 24, 2017

Uh oh!

danbev commented May 24, 2017

Uh oh!

refack left a comment

Choose a reason for hiding this comment

Uh oh!

danbev commented May 25, 2017

Uh oh!

bnoordhuis commented May 25, 2017

Uh oh!

danbev commented May 25, 2017 via email

Uh oh!

danbev commented May 25, 2017

Uh oh!

Reviewers

danbev commented May 17, 2017•
edited
Loading

refack commented May 23, 2017•
edited
Loading