-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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(cmake): add required emscripten flags #5298
fix(cmake): add required emscripten flags #5298
Conversation
Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
3430370
to
8e331a4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SGTM although I can really only contribute a question:
How/why did the workflow here pass before -fexceptions
was added?
Asking the same question from a different angle: What is fixed by adding the option? (Is that not tested here?)
It would be great if you could add a sentence or two to the PR description, to answer that question.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @henryiii! Please note the typo in the suggested CHANGELOG entry ("requried" ➡️ "required").
I forgot to remove it from the CI, it was manually added (done now). I believe pybind11 always requires exception handling enabled? |
Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
And you can now see what happens if the flag is missing in the CI history. We are going to pretend I intended to do that. :) |
Is this what you expected?
I think I understand now why it worked before (d61a69b), but TBH I don't understand why the current state of this PR produces that error. (Google has a general ban on C++ EH, with an exception for pybind11. Of course, mixups happen, and when we accidentally tried to compile pybind11 header without C++ EH enabled, we got build errors, not runtime errors.)
Yes. (Even pybind11/pytypes.h, although that almost works without C++ EH. Making that optional would be a great thing for Google, but I never got to that.) |
Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
4cac5c0
to
6a3ca2d
Compare
Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
Thanks @henryiii! |
* fix(cmake): add required emscripten flags Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com> * Update emscripten.yaml * fix(cmake): add required emscripten flags to headers target Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com> * fix(cmake): incorrect detection of Emscripten Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com> * fix(cmake): allow pybind11::headers to be modified Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com> * fix(cmake): hide a warning when building the tests standalone Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com> * fix(cmake): use explicit variable for is config Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com> * fix(cmake): go back to ALIAS target Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com> * chore: reduce overall diff Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com> * chore: reduce overall diff Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com> * chore: shorten code a bit Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com> --------- Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
IIUC, this only gets added to CMake, but not |
This would have to be cross-compilation aware. |
Ah, I suppose so then. It could get a |
@eli-schwartz Thoughts? Happy to add whatever we need here. |
Description
Putting this in for our next release. The Pyodide authors thought this was a good idea in their Discord a while back.
Suggested changelog entry:
CC @hoodmane, @agriyakhetarpal