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: Default optimize to auto, fixing target/dev_build inference for Web #94107

Merged
merged 1 commit into from
Jul 8, 2024

Conversation

akien-mga
Copy link
Member

This is a bit awkward because we can't have both a custom default value provided by each platform's get_opts, and still use some platform-agnostic "auto" behavior in SConstruct. So with this approach we go back to the 4.2 behavior where the web platform's get_opts simply overrides whatever is in optimize, unless it's specified on the command line.

I think it's an ok compromise for now, especially given how the web platform is sensitive to binary size. But it's a quirk worth knowing and documenting for people trying to debug web exports. (See also #91800.)

@dsnopek @Faless @adamscott Could also be considered to fix - #93476 by reintroducing the previous behavior, i.e. dev builds for web will still use -Os like in 4.2. Web builds with optimize=none will likely still be broken, but they're probably simply way too big for browsers to handle?

@akien-mga
Copy link
Member Author

I'll merge this already to fix the regression for the 4.3-beta3 build I want to start now.

Reviews still welcome to fix up any potential mistake or bad design decision.

@akien-mga akien-mga merged commit 82cedc8 into godotengine:master Jul 8, 2024
18 checks passed
@akien-mga akien-mga deleted the scons-optimize-auto branch July 8, 2024 23:25
Copy link
Contributor

@Repiteo Repiteo left a comment

Choose a reason for hiding this comment

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

Makes sense to me; feels right in-line with how we handle architecture.

@dsnopek
Copy link
Contributor

dsnopek commented Jul 11, 2024

Could also be considered to fix - #93476 by reintroducing the previous behavior, i.e. dev builds for web will still use -Os like in 4.2. Web builds with optimize=none will likely still be broken, but they're probably simply way too big for browsers to handle?

Yeah, this does fix #93476 too - I'll comment and close that one in a moment.

I think an argument could be made that -Os is needed for web by default even on debug builds for the reason you give above. Since it was forced to -Os for so long previously and no one complained, maybe that's a good sign?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Web builds are no longer built with -Os by default
3 participants