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: Add an option to detect C++ modules recursively #43057

Merged
merged 1 commit into from
Feb 8, 2021
Merged

SCons: Add an option to detect C++ modules recursively #43057

merged 1 commit into from
Feb 8, 2021

Conversation

Xrayez
Copy link
Contributor

@Xrayez Xrayez commented Oct 24, 2020

Closes godotengine/godot-proposals#1619.

This adds custom_modules_recursive SCons build option which allows to detect and collect all nested C++ modules which may reside in any directory specified by custom_modules option. This option is enabled by default for external modules to achieve the greatest convenience for users.

The detection logic is made to be more strict because SCSub may be used for organizing hierarchical builds within a module itself, so the existence of register_types.h and config.py is checked as well (these are all required for a C++ module to be compiled by Godot, likely throughout existence of Godot).

For performance reasons, built-in modules are not checked recursively, and there's no benefit of doing so in the first place, so this has almost no performance impact for built-in modules.

Important improvement

It's now possible to specify a directory path pointing to a single module, and it may contain nested modules which are detected recursively. This is another improvement if you just want to compile a single module alone, as compiling with custom_modules was cumbersome if you also have other modules in the same parent directory (which also may or not be compatible with current Godot source). Scripts can be further simplified by removing various module detection hacks, for example in Goost's SConstruct:

diff --git a/SConstruct b/SConstruct
index 1840490..34f988f 100644
--- a/SConstruct
+++ b/SConstruct
@@ -138,14 +138,7 @@ for arg in ARGLIST:
 
 # Link this module as a custom module.
 modules = []
-modules.append(Dir("..").abspath)
-
-# This module may provide other nested modules, just like this one.
-try:
-    modules_path = config.get_modules_path()
-    modules.append(os.path.join(Dir(".").abspath, modules_path))
-except AttributeError:
-    pass
+modules.append(Dir(".").abspath)
 
 # We use the `custom_modules` build option to build this module, 
 # but allow to specify additional modules to build along this one.
@@ -155,23 +148,6 @@ if "custom_modules" in ARGUMENTS:
 
 build_args.append("custom_modules=%s" % ",".join(modules))
 
-# We cannot link to a single module using the `custom_modules` build option,
-# so this may compile other modules which reside in the same location as this 
-# module. To prevent this, we disable all modules there, excluding this one.
-if not env["parent_modules_enabled"]:
-    DIRNAMES = 1
-    dirs = next(os.walk(Dir("..").abspath))[DIRNAMES]
-    parent_modules = []
-
-    for d in dirs:
-        if d == module_name:
-            continue
-        if os.path.exists(os.path.join(Dir("..").abspath, d, "SCsub")):
-            parent_modules.append(d)
-
-    for m in parent_modules:
-        build_args.append("module_%s_enabled=no" % m)
-
 # Optionally disable Godot's built-in modules which are non-essential in order
 # to test out this module in the engine. For more details, refer to Godot docs:
 # https://docs.godotengine.org/en/latest/development/compiling/optimizing_for_size.html

These changes are backward-compatible with Godot 3.2 (not trivially cherry-pickable).

This adds `custom_modules_recursive` which allows to detect and collect
all nested C++ modules which may reside in any directory specified by
`custom_modules` option.

The detection logic is made to be more strict because `SCSub` may be
used for organizing hierarchical builds within a module itself, so the
existence of `register_types.h` and `config.py` is checked as well
(these are all required for a C++ module to be compiled by Godot).

For performance reasons, built-in modules are not checked recursively,
and there's no benefit of doing so in the first place.

It's now possible to specify a directory path pointing to a *single*
module, as it may contain nested modules which are detected recursively.
@Calinou Calinou added enhancement topic:buildsystem cherrypick:3.x Considered for cherry-picking into a future 3.x release labels Oct 24, 2020
@Calinou Calinou added this to the 4.0 milestone Oct 24, 2020
if is_engine(child):
continue
to_search.insert(0, child)
return modules


def is_module(path):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This method can be moved to detect_modules method above as well, but technically it would break compat for 3.2.

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.

After discussion, that seems like a good improvement to the custom_modules workflow.

@akien-mga akien-mga merged commit c05d205 into godotengine:master Feb 8, 2021
@akien-mga
Copy link
Member

Thanks!

@Xrayez Xrayez deleted the custom_modules_recursive branch February 8, 2021 15:07
@akien-mga
Copy link
Member

Cherry-picked for 3.2.4.

@akien-mga akien-mga removed the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Feb 8, 2021
@akien-mga
Copy link
Member

This seems to break Python 2 support in the 3.2 branch:

SyntaxError: unqualified exec is not allowed in function 'is_engine' it is a nested function (methods.py, line 167)

@Xrayez
Copy link
Contributor Author

Xrayez commented Feb 11, 2021

I think it would be enough to just disable recursive detection of modules in Python 2, I'm not sure if there's a better way to resolve this issue...

EDIT: Nevermind, it's a syntax error, I think it cannot be worked around. I'll try to fix this.

@akien-mga
Copy link
Member

akien-mga commented Feb 11, 2021

I think we can just move those nested functions to the outer scope and it should work, even if it's less elegant? Maybe with a leading underscore or prefix to show they're not part of the public API.

For 3.2 only of course, for master we only support Python 3.

akien-mga added a commit to akien-mga/godot that referenced this pull request Feb 11, 2021
That's not a very pretty way to parse a string from a python file
though, there should definitely be cleaner and more reliable ways
to do this without using `exec()`.
@akien-mga
Copy link
Member

As mentioned in #45897, this PR doesn't seem to work for me on Linux with Python 3.8.7 and SCons 4.1.0:

$ scons p=x11 -j7 custom_modules=..
/usr/bin/scons p=x11 -j7 custom_modules=.. 2>&1 | tee -a build.log
scons: Reading SConscript files ...
  File "<string>", line 107
    raise ValueError, "invalid version number '%s'" % vstring
                    ^
SyntaxError: invalid syntax

@Xrayez
Copy link
Contributor Author

Xrayez commented Feb 11, 2021

I've tested this on SCons 4.0.1, so likely caused by SCons upgrade, strange error.

Python 3.9.1.

@akien-mga
Copy link
Member

akien-mga commented Feb 11, 2021

Same issue with SCons 4.0.1 for me.

The problem seems to be around that same is_engine() method.

I have a lot of version.py files in my .. folder:

$ find -name "version.py"
./godot-2.1/version.py
./godot-3.1/version.py
./godot-3.0/version.py
./weblate-classref/version.py
./toolchains/i686-godot-linux-gnu_sdk-buildroot/lib/python2.7/distutils/version.py
./toolchains/i686-godot-linux-gnu_sdk-buildroot/lib/python3.9/site-packages/pkg_resources/_vendor/packaging/version.py
./toolchains/i686-godot-linux-gnu_sdk-buildroot/lib/python3.9/site-packages/setuptools/version.py
./toolchains/i686-godot-linux-gnu_sdk-buildroot/lib/python3.9/site-packages/setuptools/_vendor/packaging/version.py
./toolchains/i686-godot-linux-gnu_sdk-buildroot/lib/python3.9/distutils/version.py
./toolchains/x86_64-godot-linux-gnu_sdk-buildroot/lib/python2.7/distutils/version.py
./toolchains/x86_64-godot-linux-gnu_sdk-buildroot/lib/python3.9/site-packages/pkg_resources/_vendor/packaging/version.py
./toolchains/x86_64-godot-linux-gnu_sdk-buildroot/lib/python3.9/site-packages/setuptools/version.py
./toolchains/x86_64-godot-linux-gnu_sdk-buildroot/lib/python3.9/site-packages/setuptools/_vendor/packaging/version.py
./toolchains/x86_64-godot-linux-gnu_sdk-buildroot/lib/python3.9/distutils/version.py
./godot.git/version.py
./mono/android-toolchains/arm64-v8a-api21-clang/lib/python2.7/distutils/version.py
./mono/android-toolchains/armeabi-v7a-api18-clang/lib/python2.7/distutils/version.py
./mono/android-toolchains/x86_64-api21-clang/lib/python2.7/distutils/version.py
./mono/android-toolchains/x86-api18-clang/lib/python2.7/distutils/version.py
./weblate/version.py
./android/sdk/ndk/21.3.6528147/prebuilt/linux-x86_64/lib/python2.7/distutils/version.py
./android/sdk/ndk/22.0.7026061/prebuilt/linux-x86_64/lib/python2.7/distutils/version.py
./android/sdk/ndk/22.0.7026061/toolchains/llvm/prebuilt/linux-x86_64/python3/lib/python3.8/site-packages/pip/_vendor/packaging/version.py
./android/sdk/ndk/22.0.7026061/toolchains/llvm/prebuilt/linux-x86_64/python3/lib/python3.8/site-packages/pip/_vendor/chardet/version.py
./android/sdk/ndk/22.0.7026061/toolchains/llvm/prebuilt/linux-x86_64/python3/lib/python3.8/site-packages/pip/_vendor/distlib/version.py
./android/sdk/ndk/22.0.7026061/toolchains/llvm/prebuilt/linux-x86_64/python3/lib/python3.8/site-packages/pkg_resources/_vendor/packaging/version.py
./android/sdk/ndk/22.0.7026061/toolchains/llvm/prebuilt/linux-x86_64/python3/lib/python3.8/site-packages/setuptools/version.py
./android/sdk/ndk/22.0.7026061/toolchains/llvm/prebuilt/linux-x86_64/python3/lib/python3.8/site-packages/setuptools/_vendor/packaging/version.py
./android/sdk/ndk/22.0.7026061/toolchains/llvm/prebuilt/linux-x86_64/python3/lib/python3.8/distutils/version.py
./godot-3.2/version.py
./godot-builds/godot/version.py

The current code seems to choke on some of them.

@Xrayez
Copy link
Contributor Author

Xrayez commented Feb 11, 2021

I suspect that this may be something to do with reliability of engine detection. What is your current working directory? It may detect the same "version.py" and the same "short_name", but does not necessarily mean that's from Godot. 🙂

I guess is_engine() could just be eventually simplified to check the directory name (typically "godot"). This would make it more performant and reliable, but slightly less flexible (I do have some Godot repositories checked out as "engine", and not as "godot" in fact), but that's not critical.

EDIT: but seeing your structure, this also may not be enough. I'd probably just document that "custom_modules" should point to actual modules, your current way of specifying custom_modules=.. is just for testing purposes, that's not what users would typically do I think.

akien-mga added a commit to akien-mga/godot that referenced this pull request Feb 11, 2021
`exec()` was not a good idea as it assumes a certain type of `version.py` file
similar to Godot's own file, which is not always a reliable assumption (see
godotengine#43057 (comment)).

Also restores Python 2 support for the 3.2 branch.
akien-mga added a commit that referenced this pull request Feb 12, 2021
`exec()` was not a good idea as it assumes a certain type of `version.py` file
similar to Godot's own file, which is not always a reliable assumption (see
#43057 (comment)).

Also restores Python 2 support for the 3.2 branch.

(cherry picked from commit 75910d1)
angad-k pushed a commit to angad-k/godot that referenced this pull request Feb 12, 2021
`exec()` was not a good idea as it assumes a certain type of `version.py` file
similar to Godot's own file, which is not always a reliable assumption (see
godotengine#43057 (comment)).

Also restores Python 2 support for the 3.2 branch.
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.

Detect custom C++ modules recursively
3 participants