search

godotengine / godot-cpp

C++ bindings for the Godot script API
MIT License
1.74k stars 575 forks source link

Add lto scons option #1601

Closed Ivorforce closed 5 days ago

Ivorforce commented 1 month ago

The behavior mimics almost the exact behavior (minus "progress" options) of Godot scons:

This is an active change (edit: not anymore, defaults to 'none' by default now): Once merged, all projects depending on godot-cpp (and updating the submodule) will gain LTO for release builds. I tested a similar setup in my own project, not the exact same though (because I don't have all combinations of windows, msvc, mingw and use_llvm available). However, since that it mimics what Godot does already, it should be good, given I did not make mistakes.

One important difference is that in my implementation (edit: not anymore, consistently 'none' by default now), "auto" is consistent across platforms ("full" in production, "none" in dev). I saw small, but consistent gains in all platforms, and don't see why LTO should be avoided in some platforms by default.

Ivorforce commented 1 month ago

Not sure why the windows builds are failing. I don't have this problem with any of my git runners. If this is related to the PR, we can default to no LTO on windows. Let me know what you think.

dsnopek commented 1 month ago

Code-wise, this seems to be getting there!

However:

  1. It'd be great if @Faless could give it one last review, and
  2. This will need to be squashed into a single commit before it can be merged - see https://docs.godotengine.org/en/latest/contributing/workflow/pr_workflow.html#the-interactive-rebase
Ivorforce commented 5 days ago

Right, i forgot to change the final commit message after the changes. Should be good now!

dsnopek commented 5 days ago

Thanks!