Skip to content

SCons: Add emitter to declutter build objects #101641

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

Merged

Conversation

Repiteo
Copy link
Contributor

@Repiteo Repiteo commented Jan 16, 2025

This PR aims to fix a long-standing problem annoyance with the SCons buildsystem: storing build objects in the same directory as the source code. While there is a built-in functionality that somewhat addresses this issue via VariantDir1, it's not a suitable out-of-the-box solution with the way our repo is setup, and it doesn't easily account for the files we want in the source code (eg: generated scripts). Instead, there's another built-in way of achieving this goal that does work for us: custom emitters2! With these, we can directly modify the target/source lists of a given builder, allowing us to seamlessly reroute our intermediate build files.

This implementation adds the option redirect_build_objects to the buildsystem, enabled by default. When on, all built objects/libraries are redirected to the root bin/obj/ folder automatically, preserving the passed subfolder structure. This will not apply to any builder calls passing redirect_build_objects=False (see platform/android/SCsub). The emitter has been applied to StaticObject, SharedObject, StaticLibrary, SharedLibrary, and RES — it doesn't apply to any of the other builders (e.g. generated scripts, Program), as we want to keep their output as-is.

Footnotes

  1. https://scons.org/doc/production/HTML/scons-user/ch15s04.html

  2. https://scons.org/doc/production/HTML/scons-user/ch17s06.html

@Repiteo Repiteo added this to the 4.x milestone Jan 16, 2025
@Repiteo Repiteo requested a review from a team as a code owner January 16, 2025 15:58
@Repiteo Repiteo force-pushed the scons/separate-build-dir-emitter branch from 034f3d3 to d75182e Compare January 16, 2025 16:06
@Calinou
Copy link
Member

Calinou commented Jan 16, 2025

I'm wondering if the default path should be bin/obj instead of just obj. This would make the top-level structure leaner and mimics how most CMake projects structure their build folder (their usual equivalent of our bin folder).

Example from Quake3e:

❯ tree -L 3 build
build
├── build.ninja
├── CMakeCache.txt
├── CMakeFiles
│   ├── 3.26.0-rc6
│   │   ├── CMakeCCompiler.cmake
│   │   ├── CMakeCXXCompiler.cmake
│   │   ├── ...
│   ├── 3.30.5
│   │   ├── CMakeCCompiler.cmake
│   │   ├── CMakeCXXCompiler.cmake
│   │   ├── ...
│   ├── botlib.dir
│   │   └── code
│   ├── client.dir
│   │   └── code
│   ├── cmake.check_cache
│   ├── CMakeConfigureLog.yaml
│   ├── CMakeScratch
│   ├── pkgRedirects
│   ├── q3ui.dir
│   │   └── code
│   ├── qcommon_ded.dir
│   │   └── code
│   ├── qcommon.dir
│   │   └── code
│   ├── quake3e.ded.dir
│   │   └── code
│   ├── quake3e.dir
│   │   └── code
│   ├── quake3e_opengl_x86.dir
│   │   └── code
│   ├── quake3e_vulkan_x86.dir
│   │   └── code
│   ├── rules.ninja
│   └── TargetDirectories.txt
├── cmake_install.cmake
├── quake3e
├── quake3e.ded
└── release-linux-x86_64
    ├── client
    │   ├── be_aas_bspq3.d
    │   ├── be_aas_bspq3.o
    │   ├── ...
    │   ├── vm_x86.d
    │   ├── vm_x86.o
    │   └── vorbis
    ├── ded
    │   ├── be_aas_bspq3.d
    │   ├── be_aas_bspq3.o
    │   ├── ...
    │   ├── vm_x86.d
    │   └── vm_x86.o
    ├── quake3e.ded.x64
    ├── quake3e_opengl_x86_64.so
    ├── quake3e_vulkan_x86_64.so
    ├── quake3e.x64
    ├── rend1
    │   ├── puff.d
    │   ├── puff.o
    │   ├── ...
    │   ├── tr_world.d
    │   └── tr_world.o
    ├── rend2
    │   └── glsl
    └── rendv
        ├── puff.d
        ├── puff.o
        ├── ...
        ├── vk_vbo.d
        └── vk_vbo.o

@Repiteo Repiteo force-pushed the scons/separate-build-dir-emitter branch from d75182e to 261667b Compare January 16, 2025 20:40
@luedos
Copy link

luedos commented Jan 17, 2025

This looks great! But, can you please add support for external files (files outside of godot folder)? It seems pretty easy considering any external file most likely will come from some custom module, so all we need is just specify different 'base_folder' for custom module (being the parent folder of module itself) and different relative folder within 'bin/obj'. We can do it by providing required parameters with env. Something like this should work:
SConstruct:

env.base_folder = Path(Dir(".").srcnode().path).resolve()
env.relative_intermediate_folder = ""

modules/SCsub:

# ...
original_base_path = env_modules.base_folder
original_relative_intermediate_folder = env_modules.relative_intermediate_folder

test_headers = []
# libmodule_<name>.a for each active module.
for name, path in env.module_list.items():
    env.modules_sources = []

    # Name for built-in modules, (absolute) path for custom ones.
    is_custom_module = os.path.isabs(path)
    base_path = path if is_custom_module else name
    if is_custom_module:
        env_modules.base_folder = Path(path).parent.resolve()
        env_modules.relative_intermediate_folder = "modules/"
    else:
        env_modules.base_folder = original_base_path
        env_modules.relative_intermediate_folder = original_relative_intermediate_folder

    SConscript(base_path + "/SCsub")

    lib = env_modules.add_library("module_%s" % name, env.modules_sources)
    env.Prepend(LIBS=[lib])

    if env["tests"]:
        # Lookup potential headers in `tests` subfolder.
        import glob

        module_tests = sorted(glob.glob(os.path.join(base_path, "tests", "*.h")))
        if module_tests != []:
            test_headers += module_tests
# ...

methods.py:

def redirect_emitter(target, source, env):
    """
    Emitter to automatically redirect object/library build files to the `obj` directory, retaining
    subfolder structure. Won't apply to any files outside the main directory.
    """
    if not env["redirect_build_objects"]:
        return target, source
    redirected_targets = []
    for item in env.Flatten(target):
        if isinstance(item, str):
            item = env.File(item)
        try:
            path = Path(item.get_abspath()).resolve()
            item = env.File(f"#bin/obj/{env.relative_intermediate_folder}{path.relative_to(env.base_folder)}")
        except ValueError:
            pass
        redirected_targets.append(item)
    return redirected_targets, source

This wouldn't work for everything automatically, not in the case if files of custom module will come not within the module itself, and it can be improved. But this will solve the majority of the cases, and it also will provide some control around placing intermediate files.

@Repiteo Repiteo force-pushed the scons/separate-build-dir-emitter branch from 261667b to 5fbfc44 Compare January 18, 2025 17:25
@Repiteo Repiteo force-pushed the scons/separate-build-dir-emitter branch 2 times, most recently from 81ee824 to 5c79c79 Compare March 11, 2025 23:51
@Repiteo Repiteo force-pushed the scons/separate-build-dir-emitter branch 4 times, most recently from 908139b to 858dfd5 Compare March 12, 2025 15:41
Copy link
Member

@Ivorforce Ivorforce left a comment

Choose a reason for hiding this comment

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

Code looks great!
There are some changes I can't really comment on (those scattered across the codebase), but the main one is good to go now. I have a couple more comments, but they don't need to be addressed. Just minor code style things.

I also pulled 858dfd5 and compiled the full project with it (macOS, arm64). The obj files are redirected to bin/obj (by default) as expected. The final binary built successfully. So all good, at least for a default build.

I look forward to seeing this merged!

@Repiteo Repiteo force-pushed the scons/separate-build-dir-emitter branch from 858dfd5 to 10ed66f Compare March 15, 2025 17:09
@Repiteo Repiteo modified the milestones: 4.x, 4.5 Mar 15, 2025
@Repiteo Repiteo merged commit 863a5ff into godotengine:master Mar 16, 2025
20 checks passed
@Repiteo Repiteo deleted the scons/separate-build-dir-emitter branch March 16, 2025 00:46
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.

4 participants