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

Fix windows exception handling in build #1143

Merged
merged 2 commits into from
Oct 7, 2016

Conversation

srajko
Copy link
Collaborator

@srajko srajko commented Oct 4, 2016

Even though we had the /EHsc option in the gyp file, it was not being applied correctly. With the change, the option does get applied, which also takes care of numerous build warnings like this one:

C:\Program Files (x86)\Microsoft Visual Studio 12.0\VC\include\xtree(839): warning C4530: C++ exception handler used, but unwind semantics are not enabled. Specify /EHsc (..\src\lock_master.cc) [C:\projects\nodegit\build\nodegit.vcxproj]

@saper
Copy link
Collaborator

saper commented Oct 5, 2016

You can use ExceptionHandling: Sync in gyp to indicate /EHsc

@srajko srajko force-pushed the win-exception-handling branch 2 times, most recently from 7e3532b to 26f65e1 Compare October 5, 2016 18:03
@srajko
Copy link
Collaborator Author

srajko commented Oct 5, 2016

@saper thanks for the tip, that would be cleaner but it looks like it still results in warnings for node 0.12 builds. You can compare https://ci.appveyor.com/project/TimBranyen/nodegit/build/3029 (using "ExceptionHandling": "Sync") and https://ci.appveyor.com/project/TimBranyen/nodegit/build/3031 (using "AdditionalOptions": [ "/EHsc" ]).

Maybe when we drop node 0.12?

@saper
Copy link
Collaborator

saper commented Oct 5, 2016

My mistake. The parameter should be integer, I think the value of one(1) should do it.

@saper
Copy link
Collaborator

saper commented Oct 5, 2016

/FORCE:MULTIPLE also has its gyp equivalent, ForceFileOutput.

@srajko srajko force-pushed the win-exception-handling branch from 26f65e1 to 315208f Compare October 5, 2016 20:09
@srajko
Copy link
Collaborator Author

srajko commented Oct 5, 2016

I am getting the same results with "ExceptionHandling": 1 as with "ExceptionHandling": "Sync" - the /EHsc warnings disappear for node 4, 5, and 6, but remain for 0.12. This is the build output for "ExceptionHandling": 1: https://ci.appveyor.com/project/TimBranyen/nodegit/build/3032

So both 1 and Sync have an effect for node >0.12 but not 0.12. See in contrast the build output for an unrelated PR, where the /EHsc warnings appear for all versions of node: https://ci.appveyor.com/project/TimBranyen/nodegit/build/3030

@saper
Copy link
Collaborator

saper commented Oct 6, 2016

Strange, it should not depend on the node version. I'll check when I get somewhere near Windows. In the meantime go ahead with your changes. I just figured out at node-sass we are also using /EHsc literally.

@srajko srajko force-pushed the win-exception-handling branch from 315208f to e3d5899 Compare October 7, 2016 16:18
@johnhaley81 johnhaley81 merged commit bd54fa8 into nodegit:master Oct 7, 2016
@johnhaley81 johnhaley81 deleted the win-exception-handling branch October 7, 2016 21:16
@johnhaley81 johnhaley81 changed the title Fix windows exception handling Fix windows exception handling in build Feb 6, 2017
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.

3 participants