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

Make module dependency check recursive #97813

Merged

Conversation

dustdfg
Copy link
Contributor

@dustdfg dustdfg commented Oct 4, 2024

Fixes #97812

The env.disabled_modules and methods.disable_module weren't used anywhere so the first one was repurposed and the second just deleted

@dustdfg dustdfg requested a review from a team as a code owner October 4, 2024 14:01
@AThousandShips AThousandShips added bug topic:buildsystem cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release labels Oct 4, 2024
@AThousandShips AThousandShips added this to the 4.4 milestone Oct 4, 2024
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.

The module logic in general could probably use a broad refactor; didn't realize just how messy it could get until I went to verify the changes

Nonetheless, the logic is sound and the changes work! Changing the container type should be fine, as I doubt anything is using that variable directly

The `env.disabled_modules` and `methods.disable_module` weren't used anywhere
so the first one was repurposed and the second just deleted

Signed-off-by: Yevhen Babiichuk (DustDFG) <dfgdust@gmail.com>
@dustdfg dustdfg force-pushed the module_check_dependencies_recursively branch from 1d32402 to 8e75e02 Compare October 23, 2024 05:26
@dustdfg
Copy link
Contributor Author

dustdfg commented Oct 23, 2024

The module logic in general could probably use a broad refactor; didn't realize just how messy it could get until I went to verify the changes

I would say it about half of the SCons itself.... Anyway, I don't know what messiness you noticed but I've noticed following:

  1. Disabling/enabling one module leads to rebuild all the files (by feelings it is half of the engine) which directly/transitively depend on modules/modules_enabled.gen.h

  2. module_check_dependencies is already used not as it was meant:

def module_check_dependencies(self, module):
    """
    ...
    Meant to be used in module `can_build` methods.
    ...
    """

It is used only inside main SConstruct so I assume there could be some other bugs

  1. There is some module sorting logic. I assume it is about: if A depend on B so we should build B before A. That takes into account optional dependencies but lots of the modules don't declare their optional dependencies and we doesn't see sudden build errors. Examples of such modules: navigation optionally depends on csg and gridmap; text_server_fb optionally depends on freetype, svg and msdfgen. So one of three things:
    1. I misunderstand module sorting
    2. Sorting (and subsequently optional dependencies) is useless
    3. We have hole in the bottom of the boat but it surprisingly doesn't want to sink

@Repiteo Repiteo merged commit f41ec91 into godotengine:master Oct 30, 2024
20 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Oct 30, 2024

Thanks!

@dustdfg dustdfg deleted the module_check_dependencies_recursively branch October 30, 2024 10:51
@akien-mga akien-mga removed the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Mar 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release topic:buildsystem
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Build stucks if some recursive dependencies isn't satisfied
4 participants