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] Unify tools/target build type configuration #867

Merged
merged 1 commit into from
Oct 4, 2022

Conversation

Faless
Copy link
Contributor

@Faless Faless commented Sep 24, 2022

This PR tries to unify godot-cpp scons builds with upstream godot + godotengine/godot#66242 .

Some notes and differences:

  • editor and template_debug builds are interchangeable from the library perspective, but might be different in your own library.
    The default target is for this reason template_debug.
    You can target both by just using the debug feature tag in the gdextension file.
    You can target each specifically via the editor and standalone.debug feature tags.
  • editor builds have the extra TOOLS_ENABLED define (unused by the library itself).
  • separate_debug_symbols is missing.

Draft status:

  • separate_debug_symbols is missing. Will open a separate PR.
  • demo gdextension uses template-debug targets for both editor and template-debug (do we want to split them?). I think it's good for now. We can always change this later
  • SCons: Unify tools/target build type configuration godot#66242 Merged.
  • consolidate standalone.debug to template.debug upstream? Like above, can be done later.

@Faless Faless added enhancement This is an enhancement on the current functionality waiting for Godot This issue needs a Godot Engine improvement to be solved needs testing topic:buildsystem Related to the buildsystem or CI setup labels Sep 24, 2022
@Faless Faless added this to the 4.0 milestone Sep 24, 2022
@akien-mga
Copy link
Member

DEV_ENABLED is now used in place of DEBUG_METHODS_ENABLED.

I haven't checked the PR in detail yet but this seems wrong to me. Upstream DEBUG_METHODS_ENABLED is defined when DEBUG_ENABLED is defined, it's not tied to DEV_ENABLED/dev_build.

https://github.com/godotengine/godot/blob/67f700d72686086820721f5f60418705def3b166/core/typedefs.h#L303-L305

And this is used throughout core code so code ported from upstream Godot should keep the same convention.

@Faless
Copy link
Contributor Author

Faless commented Sep 25, 2022

And this is used throughout core code so code ported from upstream Godot should keep the same convention.

Ah, my bad, I thought it was an extension specific define. I reverted the commit.

@Faless Faless marked this pull request as ready for review September 30, 2022 17:09
@Faless Faless changed the title [WIP] [SCons] Unify tools/target build type configuration [SCons] Unify tools/target build type configuration Sep 30, 2022
SConstruct Outdated Show resolved Hide resolved
SConstruct Show resolved Hide resolved
SConstruct Outdated Show resolved Hide resolved
Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Looks pretty good, left a few comments.

@Faless Faless force-pushed the build/4.x_unify_targets branch 2 times, most recently from d3401a6 to 4dad7f4 Compare October 4, 2022 14:05
Now matches Godot `master` target names and supports the same flags with
the following notable exceptions:
- The default target is "template_debug", since it's compatible with
  editor builds (and TOOLS_ENABLED is never used internally).
- separate_debug_symbols is still not supported, and will be done in a
  separate commit.
@akien-mga akien-mga merged commit 4e5d0ee into godotengine:master Oct 4, 2022
@akien-mga
Copy link
Member

Thanks!

@codecat
Copy link
Contributor

codecat commented Nov 7, 2022

In relation to this issue: codecat/godot-tbloader#36

Has anything changed that might cause template_release to link with the debug runtime on Windows?

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 waiting for Godot This issue needs a Godot Engine improvement to be solved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants