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

SCons: Disable C++ exception handling by default #1216

Merged

Conversation

akien-mga
Copy link
Member

Counterpart to godotengine/godot#80612.

Didn't test it, neither SCons nor CMake changes.

@akien-mga akien-mga added enhancement This is an enhancement on the current functionality needs testing topic:buildsystem Related to the buildsystem or CI setup labels Aug 16, 2023
@akien-mga akien-mga added this to the 4.2 milestone Aug 16, 2023
@akien-mga akien-mga requested a review from a team as a code owner August 16, 2023 09:54
@akien-mga akien-mga force-pushed the scons-disable-exception-handling branch from c32aaca to 7638b77 Compare August 16, 2023 10:32
@dsnopek
Copy link
Collaborator

dsnopek commented Aug 21, 2023

Thanks!

Given that this was merged in Godot, it makes sense that we do it here too!

The tests pass, which is good. Skimming the Godot changes, the scons changes here look comparable. The one thing I'm worried about is if it's easy enough for a GDExtension that's using godot-cpp's SConstruct file to disable "disable_exceptions", since this isn't really an end-developer setting, but something the extension author want to set. @adamscott has been adding a number of changes along this line, and I'd like to try and test this when I get a chance.

Unfortunately, I don't know cmake and can't really review those changes.

@adamscott
Copy link
Member

adamscott commented Aug 22, 2023

Here, the question is: "Are exceptions supported by gd-extension".

If so, we should make these changes optional. If not, let's go.

@dsnopek
Copy link
Collaborator

dsnopek commented Aug 22, 2023

Here, the question is: "Are exceptions supported by gd-extension".

Whether or not to use exceptions depends on the specific GDExtension. If the GDExtension is integrating a 3rd party library into Godot that requires using exceptions, then they need to be enabled. Or, if the author of the GDException just likes exceptions and wants to use them in their own code.

So, I don't think folks will really use the command line argument: the particular GDExtension either needs exceptions or it doesn't. It'll really be the GDExtension author who is deciding to enable or disable them, and so I just want to make sure it's easy enough to do when the GDExtension's SConstruct includes the godot-cpp SConstruct.

@akien-mga
Copy link
Member Author

I haven't tested it, but in theory you should be able to just do env["disable_exceptions"] = False to override what the end user may specify when compiling.

Copy link
Collaborator

@dsnopek dsnopek left a comment

Choose a reason for hiding this comment

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

I didn't test the cmake changes, but they look OK. But I did test with scons and it didn't work without making a small change - see the code comments.

I also tried putting this in a GDExtension:

env['disable_exceptions'] = False

The code in godot-cpp still compiled (after making the CXXFLAGS change) with -fno-exceptions flag, but the user code in the GDExtension didn't, which is actually probably what we want, so I think this fine in that regard.

SConstruct Outdated Show resolved Hide resolved
SConstruct Outdated Show resolved Hide resolved
@dsnopek
Copy link
Collaborator

dsnopek commented Oct 18, 2023

Fabio and I did some testing, and we think the CCFLAGS issue may be fixed now, probably by some other refactoring that we've done here. In any case, it probably still does make sense to put it in CXXFLAGS because it's a C++ specific options.

However, this will need to be rebased now that this code has moved to godotcpp.py.

@akien-mga Are you still interested in working on this one? Or, should one of us take it over?

@akien-mga
Copy link
Member Author

I can update it tomorrow. I'll also have to update upstream Godot accordingly if we change it to CXXFLAGS, to stay consistent.

@akien-mga akien-mga force-pushed the scons-disable-exception-handling branch from 7638b77 to 37af2bb Compare October 19, 2023 14:27
@akien-mga
Copy link
Member Author

Rebased. Didn't compile to test though.

@dsnopek
Copy link
Collaborator

dsnopek commented Oct 20, 2023

I tested this with scons and it does add -fno-exceptions like expected, and running with scons disable_exceptions=no works too.

However, I wasn't able to find a way to re-enable exceptions in a SConstruct that uses the godot-cpp SConstruct. So, the idea is, if a developer makes a GDExtension that needs exceptions, they'll want their SConstruct to enable them, without having to tell users to run scons disable_exceptions=no. I tried doing:

env = SConscript("godot-cpp/SConstruct")
env['disable_exceptions'] = False

... and:

env = Environment()
env['disable_exceptions'] = False
Export('env')

SConscript("godot-cpp/SConstruct")

... and a bunch of variations on that, like:

SConscript("godot-cpp/SConstruct", exports=['env'])

... and:

SConscript("godot-cpp/SConstruct", exports={'disable_exceptions': False'})

But nothing I tried allowed me to pass that option down into godot-cpp's SConstruct. Based on my earlier comment above it seems I got this to work on an earlier version? Perhaps, it's moving to the tools/godotcpp.py that's changed things?

tools/godotcpp.py Outdated Show resolved Hide resolved
@Faless
Copy link
Contributor

Faless commented Oct 20, 2023

@dsnopek the second option, i.e.:

env = Environment()
env['disable_exceptions'] = False
Export('env')

SConscript("godot-cpp/SConstruct")

should work after applying my suggestion above

@akien-mga akien-mga force-pushed the scons-disable-exception-handling branch from 37af2bb to 354764b Compare October 22, 2023 10:45
@akien-mga akien-mga force-pushed the scons-disable-exception-handling branch from 354764b to bf1c03a Compare October 22, 2023 10:45
Copy link
Collaborator

@dsnopek dsnopek left a comment

Choose a reason for hiding this comment

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

It's working great in my testing now! Thanks!

Copy link
Contributor

@Faless Faless left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@dsnopek dsnopek merged commit 379ce2b into godotengine:master Oct 22, 2023
12 checks passed
@dsnopek
Copy link
Collaborator

dsnopek commented Oct 22, 2023

Thanks!

@akien-mga akien-mga deleted the scons-disable-exception-handling branch October 22, 2023 19:00
ProbablyWorks pushed a commit to ProbablyWorks/godot that referenced this pull request Oct 22, 2023
@dsnopek
Copy link
Collaborator

dsnopek commented Oct 23, 2023

Cherry-picked for 4.1 in PR #1281

YuriSizov pushed a commit to YuriSizov/godot that referenced this pull request Oct 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement This is an enhancement on the current functionality needs testing topic:buildsystem Related to the buildsystem or CI setup
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants