Skip to content
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

Feature/openssl 1.0.1k #289

Closed
wants to merge 5 commits into from
Closed

Conversation

indutny
Copy link
Member

@indutny indutny commented Jan 11, 2015

@indutny
Copy link
Member Author

indutny commented Jan 11, 2015

Good news, they have finally fixed the EVP message!

@indutny
Copy link
Member Author

indutny commented Jan 11, 2015

Another good news: keypress patch should be no longer floated, because it is included in 1.0.1k!

@indutny
Copy link
Member Author

indutny commented Jan 11, 2015

@bnoordhuis: please check that your x32 changes are still in. I have used opensslconf from the origin/v1.x, so I assume - yes.

@bnoordhuis
Copy link
Member

Can you make the CI check this PR? I suspect it's going to break the Windows build because of the removal of (at least) x86_64-win32-masm.asm.

@indutny
Copy link
Member Author

indutny commented Jan 11, 2015

@indutny
Copy link
Member Author

indutny commented Jan 11, 2015

@bnoordhuis: appears to be building fine on windows.

@bnoordhuis
Copy link
Member

@indutny gyp is definitely complaining:

Warning: Missing input files: deps\openssl\openssl\crypto\bn\asm\x86_64-win32-masm.asm

The fact that it builds is presumably because the Windows bots build 32 bits binaries only. Maybe @rvagg can confirm.

@bnoordhuis
Copy link
Member

This PR also seems to break parallel/test-https-foafssl on Windows.

@indutny
Copy link
Member Author

indutny commented Jan 11, 2015

@bnoordhuis: pushed update for GYP.

@indutny
Copy link
Member Author

indutny commented Jan 11, 2015

@bnoordhuis : regarding foafssl - it is not completely evident to me that this is caused by this PR.

@piscisaureus
Copy link
Contributor

@indutny Would you mind also updating https://github.com/iojs/io.js/blob/v1.x/deps/openssl/asm/Makefile? I noticed some asm files er axed in this release.

@piscisaureus
Copy link
Contributor

Building x64 on windows:

openssl.lib(bn_word.obj) : error LNK2001: unresolved external symbol bn_mul_wor
ds [D:\iojs\deps\openssl\openssl-cli.vcxproj]
openssl.lib(bn_word.obj) : error LNK2001: unresolved external symbol bn_div_wor
ds [D:\iojs\deps\openssl\openssl-cli.vcxproj]
openssl.lib(bn_add.obj) : error LNK2001: unresolved external symbol bn_add_word
s [D:\iojs\deps\openssl\openssl-cli.vcxproj]
openssl.lib(bn_mont.obj) : error LNK2001: unresolved external symbol bn_mul_add
_words [D:\iojs\deps\openssl\openssl-cli.vcxproj]
openssl.lib(bn_mont.obj) : error LNK2001: unresolved external symbol bn_sub_wor
ds [D:\iojs\deps\openssl\openssl-cli.vcxproj]
openssl.lib(bn_mul.obj) : error LNK2001: unresolved external symbol bn_mul_comb
a8 [D:\iojs\deps\openssl\openssl-cli.vcxproj]
openssl.lib(bn_mul.obj) : error LNK2001: unresolved external symbol bn_mul_comb
a4 [D:\iojs\deps\openssl\openssl-cli.vcxproj]
openssl.lib(bn_sqr.obj) : error LNK2001: unresolved external symbol bn_sqr_comb
a4 [D:\iojs\deps\openssl\openssl-cli.vcxproj]
openssl.lib(bn_sqr.obj) : error LNK2001: unresolved external symbol bn_sqr_comb
a8 [D:\iojs\deps\openssl\openssl-cli.vcxproj]
openssl.lib(bn_sqr.obj) : error LNK2001: unresolved external symbol bn_sqr_word
s [D:\iojs\deps\openssl\openssl-cli.vcxproj]
D:\iojs\Release\openssl-cli.exe : fatal error LNK1120: 10 unresolved externals
[D:\iojs\deps\openssl\openssl-cli.vcxproj]

@indutny
Copy link
Member Author

indutny commented Jan 11, 2015

@piscisaureus could you please try it again with a new commit?

@piscisaureus
Copy link
Contributor

Trying

@piscisaureus
Copy link
Contributor

@indutny yes, that works

WRT that missing file - I wrote that myself one day when I was really bored: 3568edf. I don't mind axing it; since openssl now seems properly maintained I no longer feel the need to outsmart them.

@indutny
Copy link
Member Author

indutny commented Jan 11, 2015

@piscisaureus do you want me to revert it?

@piscisaureus
Copy link
Contributor

@indutny no, it works. That asm file I created was derived from the gcc asm implementation, but since that file has been updated since it seems safer to exclude my own version. Go ahead and land.

@indutny
Copy link
Member Author

indutny commented Jan 11, 2015

Ok, any objections @bnoordhuis ?

@bnoordhuis
Copy link
Member

Well, I still don't get why parallel/test-https-foafssl is failing now when it was passing before.

@indutny
Copy link
Member Author

indutny commented Jan 11, 2015

@bnoordhuis let's give it another spin on jenkins? https://jenkins-iojs.nodesource.com/job/iojs+any-pr+multi/49/

@indutny
Copy link
Member Author

indutny commented Jan 11, 2015

@bnoordhuis seems to be timeout-ing anyway. @piscisaureus does it terminate when you run it outside of the test suite? (./iojs test/parallel/test-https-foafssl.js)

@piscisaureus
Copy link
Contributor

@indutny

That'd be Release\iojs.exe test/parallel/test-https-foafssl.js and yes, it hangs.

@rvagg
Copy link
Member

rvagg commented Jan 12, 2015

brave enough to land this prior to 1.0.0 or should we hold off?

@bnoordhuis
Copy link
Member

@piscisaureus You mean it hangs with or without this PR applied? If it's without, then the failure is not a regression and this PR should be good to go.

@indutny
Copy link
Member Author

indutny commented Jan 12, 2015

Going to see it myself.

@indutny
Copy link
Member Author

indutny commented Jan 12, 2015

Stupid me, I thought that the patch was included, but it was the artifact of cherry-pick which has disappeared after cherry-pick --abort. Here is the latest build: https://jenkins-iojs.nodesource.com/job/iojs+any-pr+multi/53/

@indutny
Copy link
Member Author

indutny commented Jan 12, 2015

jenkins is good with it now. @bnoordhuis could you please land it if it LGTY?

@bnoordhuis
Copy link
Member

I carefully scrutinized each and every one of those +1,984 −5,363 changed lines. LGTM!

indutny added a commit that referenced this pull request Jan 12, 2015
PR-URL: #289
Reviewed-By: Ben Noordhuis <[email protected]>
indutny added a commit that referenced this pull request Jan 12, 2015
Backport of: openssl/openssl@5c5e7e

Original commit message:

    Fix build failure on Windows due to undefined cflags identifier

    Reviewed-by: Tim Hudson <[email protected]>

PR-URL: #289
Reviewed-By: Fedor Indutny <[email protected]>
indutny added a commit that referenced this pull request Jan 12, 2015
indutny added a commit that referenced this pull request Jan 12, 2015
@indutny
Copy link
Member Author

indutny commented Jan 12, 2015

Landed in eebdf7a, b910613, ced41b0, a76811c, 7c4a50d ! Thanks everyone!

@indutny indutny closed this Jan 12, 2015
@indutny indutny deleted the feature/openssl-1.0.1k branch January 12, 2015 18:36
jasnell pushed a commit to jasnell/node-joyent that referenced this pull request Jan 17, 2015
jasnell pushed a commit to jasnell/node-joyent that referenced this pull request Jan 17, 2015
Backport of: openssl/openssl@5c5e7e

Original commit message:

    Fix build failure on Windows due to undefined cflags identifier

    Reviewed-by: Tim Hudson <[email protected]>

PR-URL: nodejs/node#289
Reviewed-By: Fedor Indutny <[email protected]>
jasnell pushed a commit to jasnell/node-joyent that referenced this pull request Jan 17, 2015
misterdjules pushed a commit to misterdjules/node that referenced this pull request Jan 19, 2015
misterdjules pushed a commit to misterdjules/node that referenced this pull request Jan 19, 2015
misterdjules pushed a commit to misterdjules/node that referenced this pull request Jan 19, 2015
misterdjules pushed a commit to misterdjules/node that referenced this pull request Jan 20, 2015
misterdjules pushed a commit to misterdjules/node that referenced this pull request Jan 20, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants