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.

Sign up for GitHub

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

Jump to bottom

deps: MASM.UseSafeExceptionHandlers for OpenSSL #7427

Closed
wants to merge 2 commits into from
Closed

deps: MASM.UseSafeExceptionHandlers for OpenSSL #7427

wants to merge 2 commits into from

Conversation

indutny
Copy link
Member

@indutny indutny commented Jun 26, 2016

Checklist
  • make -j4 test (UNIX) or vcbuild test nosign (Windows) passes
  • a test and/or benchmark is included
  • documentation is changed or added
  • the commit message follows commit guidelines
Affected core subsystem(s)

build

Description of change

Use msvs_settings.MASM.UseSafeExceptionHandlers when building OpenSSL
assembly code on Windows. This option appends /safeseh to the list of
assembler flags when building .asm files on Windows.

Having this option in place, separate rules in masm_compile.gypi are
no longer needed.

Fix: #7426
R= @piscisaureus or @nodejs/platform-windows

@indutny
deps: MASM.UseSafeExceptionHandlers for OpenSSL
eb57503 
Use `msvs_settings.MASM.UseSafeExceptionHandlers` when building OpenSSL
assembly code on Windows. This option appends `/safeseh` to the list of
assembler flags when building `.asm` files on Windows.

Having this option in place, separate rules in `masm_compile.gypi` are
no longer needed.

Fix: nodejs#7426
@nodejs-github-bot nodejs-github-bot added the openssl Issues and PRs related to the OpenSSL dependency. label Jun 26, 2016
@indutny indutny mentioned this pull request Jun 26, 2016
Closed
@indutny indutny added windows Issues and PRs related to the Windows platform. build Issues and PRs related to build files or the CI. labels Jun 26, 2016
@indutny
Copy link
Member Author

indutny commented Jun 26, 2016

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

@indutny
Copy link
Member Author

indutny commented Jun 26, 2016

CI is green

bnoordhuis
bnoordhuis reviewed Jun 26, 2016
View reviewed changes
deps/openssl/openssl.gyp
'includes': ['masm_compile.gypi',],
'msvs_settings': {
'MASM': {
# Use /safeseh, see: 01fa5ee
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you change 01fa5ee to commit 01fa5ee?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack.

@bnoordhuis
Copy link
Member

LGTM with a suggestion.

@indutny
Copy link
Member Author

indutny commented Jun 27, 2016

@piscisaureus does this LGTY?

@jasnell
Copy link
Member

jasnell commented Jun 27, 2016

LGTM

@indutny
Copy link
Member Author

indutny commented Jun 27, 2016

May get at least one LGTM from @nodejs/platform-windows ?

@piscisaureus
Copy link
Contributor

Looks good to me, if it works.
I was unaware that gyp has built-in rules for running masm nowadays, but since this passes CI it must.
+1 to simplification any day

@indutny
Copy link
Member Author

indutny commented Jun 27, 2016

Cool, thank you!

@indutny
Copy link
Member Author

indutny commented Jun 27, 2016

Landed in 2787d70, thank you everyone!

@indutny indutny closed this Jun 27, 2016
@indutny indutny deleted the ref/7426 branch June 27, 2016 18:11
indutny added a commit that referenced this pull request Jun 27, 2016
@indutny
deps: MASM.UseSafeExceptionHandlers for OpenSSL
2787d70 
Use `msvs_settings.MASM.UseSafeExceptionHandlers` when building OpenSSL
assembly code on Windows. This option appends `/safeseh` to the list of
assembler flags when building `.asm` files on Windows.

Having this option in place, separate rules in `masm_compile.gypi` are
no longer needed.

Fix: #7426
PR-URL: #7427
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Bert Belder <bertbelder@gmail.com>
Fishrock123 pushed a commit that referenced this pull request Jul 5, 2016
@indutny @Fishrock123
deps: MASM.UseSafeExceptionHandlers for OpenSSL
a4880b5 
Use `msvs_settings.MASM.UseSafeExceptionHandlers` when building OpenSSL
assembly code on Windows. This option appends `/safeseh` to the list of
assembler flags when building `.asm` files on Windows.

Having this option in place, separate rules in `masm_compile.gypi` are
no longer needed.

Fix: #7426
PR-URL: #7427
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Bert Belder <bertbelder@gmail.com>
@Fishrock123 Fishrock123 mentioned this pull request Jul 5, 2016
Merged
@MylesBorins MylesBorins mentioned this pull request Jul 11, 2016
Closed
@MylesBorins
Copy link
Contributor

@indutny lts?

@indutny
Copy link
Member Author

indutny commented Jul 12, 2016

I think - yes.

@richardlau
Copy link
Member

I think this is now causing warnings on 64-bit Windows:
e.g. https://ci.nodejs.org/job/node-compile-windows/3009/

MASM : warning A4018: invalid command-line option : /safeseh [c:\workspace\node-compile-windows\label\win-vs2015\deps\openssl\openssl.vcxproj]

In the now removed deps/openssl/masm_compile.gypi, /safeseh was only passed through to ml.exe when 'target_arch=="ia32"'. /safeseh is Not available in ml64.exe.

@indutny
Copy link
Member Author

indutny commented Jul 16, 2016

@richardlau thank you! Opened #7759 to fix this.

@indutny indutny mentioned this pull request Jul 16, 2016
Closed
4 tasks
@indutny
Copy link
Member Author

indutny commented Jul 17, 2016

Should be fixed now, @richardlau ! Please give it a try.

@MylesBorins
Copy link
Contributor

@indutny would you be willing to make a backport that collects this and the fix?

@indutny
Copy link
Member Author

indutny commented Aug 30, 2016

@thealphanerd yep, to v4?

indutny added a commit to indutny/io.js that referenced this pull request Sep 3, 2016
@indutny
deps: MASM.UseSafeExceptionHandlers for OpenSSL
72a08e2 
Use `msvs_settings.MASM.UseSafeExceptionHandlers` when building OpenSSL
assembly code on Windows. This option appends `/safeseh` to the list of
assembler flags when building `.asm` files on Windows.

Having this option in place, separate rules in `masm_compile.gypi` are
no longer needed.

Fix: nodejs#7426
PR-URL: nodejs#7427
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Bert Belder <bertbelder@gmail.com>
@indutny
Copy link
Member Author

indutny commented Sep 3, 2016

@thealphanerd here you go #8399

MylesBorins pushed a commit that referenced this pull request Sep 4, 2016
@indutny @MylesBorins
deps: MASM.UseSafeExceptionHandlers for OpenSSL
ef977f6 
Use `msvs_settings.MASM.UseSafeExceptionHandlers` when building OpenSSL
assembly code on Windows. This option appends `/safeseh` to the list of
assembler flags when building `.asm` files on Windows.

Having this option in place, separate rules in `masm_compile.gypi` are
no longer needed.

Fix: #7426
PR-URL: #7427
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Bert Belder <bertbelder@gmail.com>
MylesBorins pushed a commit that referenced this pull request Sep 28, 2016
@indutny
deps: MASM.UseSafeExceptionHandlers for OpenSSL
77827eb 
Use `msvs_settings.MASM.UseSafeExceptionHandlers` when building OpenSSL
assembly code on Windows. This option appends `/safeseh` to the list of
assembler flags when building `.asm` files on Windows.

Having this option in place, separate rules in `masm_compile.gypi` are
no longer needed.

Fix: #7426
PR-URL: #7427
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Bert Belder <bertbelder@gmail.com>
rvagg pushed a commit that referenced this pull request Oct 18, 2016
@indutny @rvagg
deps: MASM.UseSafeExceptionHandlers for OpenSSL
0e1162c 
Use `msvs_settings.MASM.UseSafeExceptionHandlers` when building OpenSSL
assembly code on Windows. This option appends `/safeseh` to the list of
assembler flags when building `.asm` files on Windows.

Having this option in place, separate rules in `masm_compile.gypi` are
no longer needed.

Fix: #7426
PR-URL: #7427
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Bert Belder <bertbelder@gmail.com>
MylesBorins pushed a commit that referenced this pull request Oct 26, 2016
@indutny @MylesBorins
deps: MASM.UseSafeExceptionHandlers for OpenSSL
ea36c61 
Use `msvs_settings.MASM.UseSafeExceptionHandlers` when building OpenSSL
assembly code on Windows. This option appends `/safeseh` to the list of
assembler flags when building `.asm` files on Windows.

Having this option in place, separate rules in `masm_compile.gypi` are
no longer needed.

Fix: #7426
PR-URL: #7427
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Bert Belder <bertbelder@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Oct 26, 2016
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
build Issues and PRs related to build files or the CI. openssl Issues and PRs related to the OpenSSL dependency. windows Issues and PRs related to the Windows platform.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

OpenSSL windows asm question
7 participants
@indutny @bnoordhuis @jasnell @piscisaureus @MylesBorins @richardlau @nodejs-github-bot